Closed Bug 1475589 Opened 6 years ago Closed 6 years ago

formatValue in URL bar doesn't get re-run when dragging tab to new window

Categories

(Firefox :: Address Bar, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- verified
firefox63 --- verified

People

(Reporter: Gijs, Assigned: florian)

References

Details

(Keywords: regression, Whiteboard: [fxsearch])

Attachments

(1 file)

STR:

1. open https://www.mozilla.org/
2. (note "mozilla.org" is highlighted in URL bar, everything else has lower opacity text)
3. open another tab in that window if this is the only tab
4. switch back to mozilla.org tab and drag it into its own window

ER:
URL formatting is the same as before

AR:
No more URL formatting

Bonus extra: if you drag the tab back to its original window, the formatting returns in that URL bar. Magic!
58 was also affected... but 52 wasn't.
Seems this was caused by bug 1391704.

Florian, can you take a look? The URL bar formatting is supposed to help convey security information to the user so ideally we shouldn't break it. This may be related to some of the other textbox/focus regressions that have been recently filed against bug 1391704...
Blocks: 1391704
Flags: needinfo?(florian)
Priority: -- → P2
Whiteboard: [fxsearch]
Blocks: 1425024
(In reply to :Gijs (he/him) from comment #2)

> This may be related to some of the other textbox/focus regressions that have
> been recently filed against bug 1391704...

It's not related to bug 1459327 (I'm assuming this is what you had in mind).

The problem here is a bad interaction with the hacks I introduced in bug 1403648 that add the "focused" attribute by default on the urlbar and remove it later if we aren't sure the urlbar will actually be focused.

The formatValue code returns early if the urlbar is focused, and that's what's happening here.

My first idea for a trivial fix was to add gURLBar.formatValue(); at https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/browser/base/content/browser.js#1646 This works, but somehow the urlbar focus ring still flickers for a frame or so.

I think a better fix is to remove the "focused" attribute early in this case, because we know the urlbar won't be focused when we are adopting a tab.
Assignee: nobody → florian
Flags: needinfo?(florian)
Attached patch PatchSplinter Review
Attachment #8995533 - Flags: review?(gijskruitbosch+bugs)
Attachment #8995533 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15f7f56e614d
remove the focused attribute early on the urlbar when adopting a tab, to ensure the url gets properly formatted, r=Gijs.
https://hg.mozilla.org/mozilla-central/rev/15f7f56e614d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Doesn't seem impactful enough to worry about for ESR60, but did you want to nominate this for Beta consideration?
Flags: needinfo?(florian)
Comment on attachment 8995533 [details] [diff] [review]
Patch

(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)
> Doesn't seem impactful enough to worry about for ESR60, but did you want to
> nominate this for Beta consideration?

Given that this regressed in 57 and only got noticed about a month ago, I don't think there's any urgency to fix this, but on the other hand the fix is very low risk and as Gijs mentioned in comment 2, this is supposed to be a security feature, so it's better to not break it.

I'm adding a beta approval request, but I don't mind either way.

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: Broken coloration of the hostname in the urlbar of detached tabs.
[Is this code covered by automated tests?]: no.
[Has the fix been verified in Nightly?]: Doesn't look like it.
[Needs manual test from QE? If yes, steps to reproduce]: steps in comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no.
[Why is the change risky/not risky?]: Just removing the focus from the urlbar earlier, only in the specific case of detaching a tab.
[String changes made/needed]: none
Flags: needinfo?(florian)
Attachment #8995533 - Flags: approval-mozilla-beta?
Comment on attachment 8995533 [details] [diff] [review]
Patch

Seems like a reasonably low risk fix. Let's uplift for beta 17.
Attachment #8995533 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
I could reproduce this issue (comment 0) on 
Build ID 	20180704003137
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0

I tested this bug fix on following

Build ID 	20180814100100
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0

Version 	62.0b17
Build ID 	20180813190114
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0

User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:63.0) Gecko/20100101 Firefox/63.0

User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:62.0) Gecko/20100101 Firefox/62.0

User Agent 	Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:63.0) Gecko/20100101 Firefox/63.0

User Agent 	Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:62.0) Gecko/20100101 Firefox/62.0

URL formatting is working as expected when moving from one window to another/or dragging back to original  window.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: