893312 - Location Bar scrolled when loading a long URL

Status: VERIFIED WORKSFORME

(Firefox :: Address Bar, defect, P3)

Description

[Alice0775 White]

Attachment: screenshot

Steps To Reproduced:
1. Copy the long url
   Ex. https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-aurora/?dummy=111111111111111111111111111111111111111111111
2. Right click on Location Bar and Choose "Paste & Go"

Actual Results:
Location Bar scrolled to random position.
The pasted URL displayed from random(?) position.

Expected Results:
Location Bar should not scroll. 
The pasted URL should display from beginning(or trim http://)


Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/0a91da5f5eab
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130227 Firefox/22.0 ID:20130227052338
Bad:
http://hg.mozilla.org/mozilla-central/rev/8cb9d6981978
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130227 Firefox/22.0 ID:20130227173648
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0a91da5f5eab&tochange=8cb9d6981978


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/0a91da5f5eab
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130227 Firefox/22.0 ID:20130227045336
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ef0c622197dd
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130227 Firefox/22.0 ID:20130227061947
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0a91da5f5eab&tochange=ef0c622197dd


Regressed By:
ebbcf3fc9240	Mounir Lamouri — Bug 230474 - Bug 829606 - No longer scroll to the beginning of the editor when the value is changed in a text field. r=bz

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch

Should fix the bug.

Comment 2

[Alice0775 White]

WAIT!

This is not only "Paste & Go" but also Press Enter/Click Go Button/Reload.
And I confirm that this is also same regression range(caused by Bug 829606).

Another STR
1. Open long URL
   Ex. https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-aurora/?dummy=111111111111111111111111111111111111111111111
2. Scroll text of URL to right
3. Then, Press Enter/Click Go button/Reload

Actual Results:
  Page loaded.
  URLbar not scroll to beginning

Expected Results:
  Page loaded.
  The URL should display from beginning(and trimed http://, formatted domain)

Comment 3

[Brian R. Bondy [:bbondy]]

Comment on attachment 775978 [details] [diff] [review]
Patch

Thanks for also applying a fix on the Metro side, but cancelling the review request based on the last comment which indicates that the issue is not only with Paste & Go.

Comment 4

[Mounir Lamouri (:mounir)]

All the call points that set an URL should set the selection back to the beginning of the URL bar.

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 775978 [details] [diff] [review]
Patch

What about taking that patch for "Paste and Go" and fix the other call points later?

Comment 6

[Brian R. Bondy [:bbondy]]

Comment on attachment 775978 [details] [diff] [review]
Patch

That doesn't sound like a good idea to me, paste and go is not the most common operation and I don't think special casing its functionality is the best route. I'd prefer a fix in content for this, or if that's not possible for some reason then I'd prefer to stay consistent with all ways we load URLs.  Fixing it all at once and remaining consistent would be best.

This is also something we should have a test for.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mounir Lamouri (:mounir) from comment #4)
> All the call points that set an URL should set the selection back to the
> beginning of the URL bar.

That doesn't seem reasonable. How about a way for chrome to opt in to the pre-bug 829606 behavior?

Comment 8

[Mounir Lamouri (:mounir)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #7)
> That doesn't seem reasonable. How about a way for chrome to opt in to the
> pre-bug 829606 behavior?

Given how easy it is for any code to move the selection, special casing does not seem a good idea (cost in code complexity and maintainability mostly).

I am unassigning myself and will let the Firefox team to take care of this.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mounir Lamouri (:mounir) from comment #8)
> Given how easy it is for any code to move the selection, special casing does
> not seem a good idea (cost in code complexity and maintainability mostly).

I don't really understand. Making the behavior optional (either by pref for chrome or content attribute or something) seems not that difficult, and the regression from a user point of view seems important.

> I am unassigning myself and will let the Firefox team to take care of this.

Disclaiming responsibility for a regression you caused isn't ideal :) Should we just back out bug 829606?

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #9)
> (In reply to Mounir Lamouri (:mounir) from comment #8)
> > Given how easy it is for any code to move the selection, special casing does
> > not seem a good idea (cost in code complexity and maintainability mostly).
> 
> I don't really understand. Making the behavior optional (either by pref for
> chrome or content attribute or something) seems not that difficult, and the
> regression from a user point of view seems important.

This would indeed not be difficult. But I am thinking of the long term here and adding code paths like that in the backend for features that would apply only to chrome code but not content is not ideal. I would rather prefer the chrome code to solve this by changing the selection when needed. Is it possible to refactor the code so whenever the URL is changed, the same method is called and that method would take care of calling editor.beginningOfDocument()?

> > I am unassigning myself and will let the Firefox team to take care of this.
> 
> Disclaiming responsibility for a regression you caused isn't ideal :) Should
> we just back out bug 829606?

I am not disclaiming responsibility. As you can see I jumped on that bug to fix it. However, I do not have enough cycles to look for all the places where the URL bar is changed in Firefox and make sure the selection is moved to the beginning of the text field. A Firefox team member would do that significantly faster than I would. I could have stayed assigned and try to look into it when I will have time but that did not sound the right thing to do.

This behaviour has been changed because content required it and as far as I remember, there was a web compatibility issue here (though, tiny, I agree). I am not sure if it is better to break content in favour of chrome code.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

The URL bar value is set in a lot of ways, and it doesn't seem realistic to find all of those places and make them also scroll the textbox. URL bars want this behavior consistently. An attribute similar to nsIPlaintextEditor's "newlineHandling" seems better.

Comment 13

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 14

[:Gijs (he/him)]

Pretty sure this is WFM at this point, both before and after bug 1419391. Marco, can you confirm?

Comment 15

[Alice0775 White]

verified to fix on Nightly63