1605889 - Reset address bar scroll position on blur to show LTR origins

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P3)

Description

[Itiel]

STR:

1. Overflow the urlbar with a long url, hit enter
2. Click the urlbar, hit End
3. Unfocus the urlbar

AR:
URL is scrolled to the end, and the domain part is hidden

ER:
URL gets scrolled back to the origin, and the domain part is displayed.

Regressed by bug 1593964, mozregression claims part 3 is the exact regressor.

Comment 1

[Dão Gottwald [:dao]]

Can you reproduce with the megabar disabled?

Comment 2

[Itiel]

(In reply to Dão Gottwald [::dao] from comment #1)

Can you reproduce with the megabar disabled?

By toggling browser.urlbar.update1 to false and restarting? Yes.

Comment 3

[Dão Gottwald [:dao]]

(In reply to Itiel from comment #0)

Regressed by bug 1593964, mozregression claims part 3 is the exact regressor.

Marco, can you take a look?

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Sorry, missed that comment 2 suggests this reproduces with megabar disabled too.

Comment 5

[Marco Bonardo [:mak]]

I'll have a look, but I can't reproduce atm, unless AR and ER are inverted or I missed something.

Is this with an RTL build, with an RTL domain?

Comment 6

[Itiel]

(In reply to Marco Bonardo [:mak] from comment #5)

unless AR and ER are inverted or I missed something.

Ah yes, that was the case. Fixed now.

Is this with an RTL build, with an RTL domain?

Doesn't matter if the domain is RTL.
IIRC this is an issue regardless of RTL/LTR directionality.

Comment 7

[Marco Bonardo [:mak]]

I'm still investigating reasons, but I must specify that it was not part 3 regressing this, it was part 1... That makes sense, I moved some selection code there, the reasons that code was added were not related, but it looks like it ended up supporting this case. I'll revise those changes and find a better call point for them.

Comment 8

[Marco Bonardo [:mak]]

Ok, I didn't technically regress this, what happened is that Bug 1571018 inadvertenly made us set the selection to zero on blur, with the scope of clearing an existing selection, not certainly to ensure the origin stays visible. That ended up "fixing" this bug. When I moved that code to a better position for its scope, in bug 1593964, I removed the accidental fix for this bug.
I went back to a version in May 2019 and even then we didn't use to move the selection.

I don't know what's the expected behavior here, we surely care that the origin is visible at least once, and that already happens on load.
I suspect we didn't change the selection on blur to avoid too much visual jumping, though while this was in the tree nobody really complained so... I guess we could fix it properly.

Comment 9

[Dão Gottwald [:dao]]

I think the current behavior makes sense for people who might want to edit, blur, and then continue to edit for whatever reason. There doesn't seem to be a strong case for resetting the scroll position on blur so I think we could just wontfix this.

Comment 10

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #9)

I think the current behavior makes sense for people who might want to edit, blur, and then continue to edit for whatever reason. There doesn't seem to be a strong case for resetting the scroll position on blur so I think we could just wontfix this.

Bug 1593964 comment 9 claims there's a security issue but I'm not sure I'd frame it that way.

Comment 11

[Marco Bonardo [:mak]]

(In reply to Dão Gottwald [::dao] from comment #9)

I think the current behavior makes sense for people who might want to edit, blur, and then continue to edit for whatever reason. There doesn't seem to be a strong case for resetting the scroll position on blur so I think we could just wontfix this.

Both sides have their pro and cons. I thought about the "continue editing" case, but if you use the keyboard shortcut we select the whole url (resetting the selection) and if you click on the urlbar, we either select all (still losing your position) or put the cursor where you click. The latter case happens only on Linux (provided we're not going to clickSelectsAll there, as discussed many times) or profiles with disabled clickSelectsAll, plus this would only affect cases where you are editing a long url at the end. So it's an edge case.

Moreover, for urls with RTL domains the formatter already ensures that the domain stays visible, so fixing this would make the behavior more coherent.
Considered that this new behavior has been in the product for months without negative feedback, it slightly improves security by keeping the origin visible, and it's coherent with RTL domains mode, I think we should fix this. There seems to be a little bit more benefits than downsides.

Comment 12

[Marco Bonardo [:mak]]

I also checked what other browsers do, both Edge and Chrome seem to act like it's requested here (reset scroll position on blur).

Comment 13

[Marco Bonardo [:mak]]

Attachment: Bug 1605889 - Ensure LTR origins stay visible on urlbar blur. r=dao

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c95e424797ec
Ensure LTR origins stay visible on urlbar blur. r=dao

Comment 15

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/c95e424797ec

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Thanks for verifying this, Itiel. Mak, please nominate this for Beta approval when you get a chance.

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 9121293 [details]
Bug 1605889 - Ensure LTR origins stay visible on urlbar blur. r=dao

Beta/Release Uplift Approval Request

• User impact if declined: On blur we don't reset the urlbar input field scroll position to keep the origin in sight.
  This patch reverts that behavior (reset scroll position) that we already had "by accident" in previous version, and that is on parity with other browsers
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 0 has steps
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): it's mirroring what we already do for RTL domains, single line code change with test.
• String changes made/needed:

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9121293 [details]
Bug 1605889 - Ensure LTR origins stay visible on urlbar blur. r=dao

Fixes an accidental change in behavior with the location bar and provides more consistent behavior with other browsers. Approved for 73.0b8.

Comment 19

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/21851727fe14

Comment 20

[Daniel Bodea [:danibodea]]

Steps to reproduce are vague. I have noticed the difference between the affected and fixed builds and I have used these steps:

1. Open the browser and load this test page:
   'http://chart.apis.google.com/chart?chs=500x500&chma=0,0,100,100&cht=p&chco=FF0000%2CFFFF00%7CFF8000%2C00FF00%7C00FF00%2C0000FF&chd=t%3A122%2C42%2C17%2C10%2C8%2C7%2C7%2C7%2C7%2C6%2C6%2C6%2C6%2C5%2C5&chl=122%7C42%7C17%7C10%7C8%7C7%7C7%7C7%7C7%7C6%7C6%7C6%7C6%7C5%7C5&chdl=android%7Cjava%7Cstack-trace%7Cbroadcastreceiver%7Candroid-ndk%7Cuser-agent%7Candroid-webview%7Cwebview%7Cbackground%7Cmultithreading%7Candroid-source%7Csms%7Cadb%7Csollections%7Cactivity|Chart'
2. Click the link in the address bar to select it whole.
3. Tap the right arrow key from the keyboard to go to the end of the link.
4. Deselect the link/address bar by chicking anywhere else on the content area of the page.
   Affected: The link will remain displayed alligned to the right.
   Fixed: The link will reset to the normal left allignment.

With these steps I have verified this fix in Nightly v74.0a1 from 2020-01-20 and Beta v73.0b8 on Windows 10 and Mac OS 10.14.
Please confirm that these steps are valid to verify the fix!

Comment 21

[Marco Bonardo [:mak]]

Yep, looks good to me. Thank you.