Closed Bug 893312 Opened 7 years ago Closed Last year

Location Bar scrolled when loading a long URL

Categories

(Firefox :: Address Bar, defect, P3)

22 Branch
x86_64
All
defect
Points:
8

Tracking

()

VERIFIED WORKSFORME
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- wontfix
firefox25 --- wontfix
firefox-esr17 --- unaffected

People

(Reporter: alice0775, Unassigned)

References

Details

(Keywords: csectype-spoof, regression, sec-low)

Attachments

(2 files)

Attached image 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
Attached patch PatchSplinter Review
Should fix the bug.
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #775978 - Flags: review?(netzen)
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 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.
Attachment #775978 - Flags: review?(netzen)
Summary: Location Bar scrolled when I execute "Paste & Go" with long url → Location Bar scrolled when loading a long URL
All the call points that set an URL should set the selection back to the beginning of the URL bar.
Comment on attachment 775978 [details] [diff] [review]
Patch

What about taking that patch for "Paste and Go" and fix the other call points later?
Attachment #775978 - Flags: review?(netzen)
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.
Attachment #775978 - Flags: review?(netzen) → review-
(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?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Assignee: mounir → nobody
(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?
Duplicate of this bug: 903898
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
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.
Whiteboard: p=0
No longer blocks: fxdesktopbacklog
Status: ASSIGNED → NEW
Flags: firefox-backlog+
Whiteboard: p=0 → p=8
Points: --- → 8
Whiteboard: p=8
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.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INACTIVE
Status: RESOLVED → REOPENED
Priority: -- → P3
Resolution: INACTIVE → ---
Pretty sure this is WFM at this point, both before and after bug 1419391. Marco, can you confirm?
Status: REOPENED → RESOLVED
Closed: 2 years agoLast year
Flags: needinfo?(mak77)
Resolution: --- → WORKSFORME
verified to fix on Nightly63
Flags: needinfo?(mak77)
You need to log in before you can comment on or make changes to this bug.