Closed Bug 1767053 Opened 2 years ago Closed 2 years ago

Raw Json viewer displays unreadable address

Categories

(Firefox :: Address Bar, defect)

Firefox 101
Desktop
All
defect

Tracking

()

VERIFIED FIXED
102 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox99 --- unaffected
firefox100 --- unaffected
firefox101 --- wontfix
firefox102 --- verified

People

(Reporter: aflorinescu, Assigned: daisuke)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Attached image rawjson_address.jpg

Affected versions

  • Nightly 101

Affected platforms

  • all , but in a different way:

    * Windows : end of the json address is displayed
    * Ubuntu 22: end of the json address  is displayed
    * Mac 11: end of the json address  is displayed
    

Steps to reproduce

  1. Open new profile.
  2. Open about:telemetry
  3. Left bottom corner, press Raw Json.

Expected result
Raw json's address is readable

Actual result
Raw json's address is unreadable

Regression range

  • Found commit message: Bug 1764411 - Cleanup toolkit/components/url-classifier/ includes

Note
Not sure what the effects outside of this scenario are.

:serge.guelton, since you are the author of the regressor, bug 1764411, could you take a look?
For more information, please visit auto_nag documentation.

Flags: needinfo?(serge.guelton)
Has STR: --- → yes

Set release status flags based on info from the regressing bug 1764411

The reported regression range didn't seem plausible to me, so I took a shot at bisecting it myself and ended up at bug 1674951 as the regressor, which seems more likely? Can you please confirm that, Adrian?

Flags: needinfo?(adrian.florinescu)

Thanks Ryan for taking time to investigate this, bug 1674951 as regressor is correct. Updating the sumarry/description to reflect the bug that is windows/mac reproducible (the json string address is not correctly positioned), for which also my windows regression range reliably leads to Bug 1674951: Update the caret position after loading page. r=adw

However, there is a 2nd regression or behavior (which messed up the original regression range), which is mac specific and hitting it requires an intermediate step: perform an address bar search, in which case the json address on mac will look like in the bottom part of the original screenshot(blanked out). So far, didn't manage to get something probable as a regression range for that part, but the reason for which I didn't rush to log a spin'off bug is because the regression range for this part is really close to the bug 1674951 one. I'll get back to this 2nd part on Monday.

Component: Safe Browsing → Address Bar
Flags: needinfo?(serge.guelton)
Flags: needinfo?(daisuke)
Flags: needinfo?(adrian.florinescu)
Product: Toolkit → Firefox
No longer regressed by: 1764411
Regressions: 1674951
Summary: Raw Json viewer displays unreadable address (depends on OS) → Raw Json viewer displays unreadable address
Flags: needinfo?(adrian.florinescu)

Bug 1674951 should be in the regressed by field.

Flags: needinfo?(adrian.florinescu)
Regressed by: 1674951
No longer regressions: 1674951
See Also: → 1768401

(In reply to Adrian Florinescu [:aflorinescu] from comment #4)

However, there is a 2nd regression or behavior (which messed up the original regression range), which is mac specific and hitting it requires an intermediate step: perform an address bar search, in which case the json address on mac will look like in the bottom part of the original screenshot(blanked out). So far, didn't manage to get something probable as a regression range for that part, but the reason for which I didn't rush to log a spin'off bug is because the regression range for this part is really close to the bug 1674951 one. I'll get back to this 2nd part on Monday.

Redoing the regression range with the added steps for Mac, still lead me to bug 1764411. Logged the spinoff for mac separately, bug 1768401.

I also could reproduce this issue. I will fix it.

Drew, I'd like to hear your opinion.
If we set 0 to the start/end caret position explicitly, it seems that the issue is fixed at least in my env.
So, to resolve this issue, why don't we make the caret position 0 when one of the following cases?

  • If the new or previous value does not indicate http[s] protocol.
  • If the new value's protocol does not match the previous' protocol.
    What do you think?
Flags: needinfo?(daisuke) → needinfo?(adw)

(In reply to Daisuke Akatsuka (:daisuke) from comment #7)

I also could reproduce this issue. I will fix it.

Thanks Daisuke.

If we set 0 to the start/end caret position explicitly, it seems that the issue is fixed at least in my env.

That makes sense because before bug 1674951 the caret index was always set to 0.

So, to resolve this issue, why don't we make the caret position 0 when one of the following cases?

  • If the new or previous value does not indicate http[s] protocol.
  • If the new value's protocol does not match the previous' protocol.
    What do you think?

I'm not sure about that. First, we should understand why this is happening. I'm interested in knowing which code paths get hit in the STR in comment 0, and what the values of the relevant variables are. Could you investigate that please?

I did some quick debugging and found that when I click the Raw JSON link, setURI() is called 3 times with these variable values:

  1. isDifferentValidValue=false
  2. isDifferentValidValue=false
  3. isDifferentValidValue=true
    isCaretPositionEnd=true
    previousSelectionStart=11
    previousSelectionEnd=11
    

I didn't log the values being set, which would have been helpful. But why is it called 3 times, why is isDifferentValidValue=true the third time, why is the caret index 11, etc.

Flags: needinfo?(adw)

Thanks Drew.

I have investigated this.
When clicking “Row JSON” button, we call openJsonInFirefoxJsonViewer(). And, it seems that it calls setURI() totally 3 times inside via onLocationChange() in browser.js.

The first time, when a newly opened tab is detected as selected, it is explicitly. The target url is about:blank. The variables of previousSelectionStart/previousSelectionEnd are 15 that is length of about:telemetry.
The second time, when tabtowser.js detects location change event for about:blank via progress listener, then calls onLocationChange of browser.js. The target url is also about:blank. The variables of previousSelectionStart/previousSelectionEnd are 11 that is length of about:blank.
The third time, when tabtowser.js detects location change event for the data URL of JSON via progress listener at the same place of above. The target url is the data url. The variables of previousSelectionStart/previousSelectionEnd are 11 that is length of about:blank. And, in this case, as previousSelectionEnd is as same as the length of about:blank, isCaretPositionEnd will be true, and the caret comes to move to end. It seems this is a cause.

The solution I am thinking now is:

  1. When opening a new tab or restoring a tab(= !valid), make the caret position 0.
  2. Furthermore, in the current algorithm, if the string length is 0 and previousSelectionEnd is also 0, isCaretPositionEnd will be true, the caret comes to move to end. I think we should keep the caret position 0 in this case.

What do you think?
I will try to write a patch with the way once and will check whether there are test failures by the solution.

It seems that only browser_retainedResultsOnFocus.js test fails.

That sounds good to me. Have you been able to figure out the test failure? Thanks for investigating and sorry for the delay.

Assignee: nobody → daisuke
Status: NEW → ASSIGNED
Attachment #9276631 - Attachment description: Bug 1767053: Reset caret position when invalid and keep the position when the previous end caret position is 0. → Bug 1767053: Change the logic for taking over the caret position.
Pushed by dakatsuka.birchill@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fbd7e82e204f
Change the logic for taking over the caret position. r=adw
Regressions: 1770092
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 102 Branch

Verified as fixed using Firefox 102 beta 5 under Win 10 64-bit, Mac OSX 12.3 and Ubuntu 22.04. Filled bug 1773333 as a follow-up for OSX only.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: