1921811 - Make urlbar a popover so that it draws on the top layer

Status: REOPENED

(Firefox :: Address Bar, defect, P3)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: WIP: Bug 1921811 - Make URLBar a popover. r=emilio

Main issue is:

• #navigator-toolbox was relpos only on macOS, making the z-index work.
• We generally do want the toolbox to draw on top of the content for
  the urlbar breakout mechanism, and for macOS' full-screen toolbar
  animation.
• However, for sidebar.revamp, where the content separator is an
  outline + shadow, we want that outline + shadow to draw on top.

This patch fixes the urlbar case by making it a popover (and thus moves
the urlbar to the top layer, outside the toolbox).

The full-screen animation thing I think we want to fix in a different
way (much like Chrome and Safari do, where the titlebar buttons just
fade in when the menubar is shown).

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Dao, Sam... So this is hitting again the paint ordering issue Sam was hitting in bug 1899598 where we really want the toolbox to be drawn under the content, but that breaks the urlbar breakout and the toolbox full-screen animation on macOS.

I think however both are kinda fixable:

• The urlbar can be made a popover and thus moved to the top layer (comment 1 prototypes that).
• The toolbox animation we can probably change to work more like in Chrome / Safari where, instead of shifting their toolbars, they fade in their titlebar buttons. That would also fix bug 1921819.

Does such a plan look good? I asked sheriffs to back out the regressing change from beta tho, since this bug seems worse than what the regressing bug was fixing.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Screenshot on this bug due to the toolbox painting over the content shadow / outline

Comment 4

[Sarah Clements [:sclements]]

So if this is a regression caused by bug 1921257, and that bug had the patch backed out of 132, then is this issue only on 133 Nightly?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Yes.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

This got "fixed" by backing out bug 1921257, but this is probably still going to bite us in the future.

We should consider doing something like comment 1, not canceling the ni?s because they're still relevant.

Comment 8

[Sam Foster [:sfoster] (he/him)]

Clearing my need-info here. The plan in comment 2 and the WIP patch both look reasonable to me, but I don't have any context for the current implementation of the urlbar and its results view, and if there were requirements or circumstances that would preclude this solution.

Comment 9

[Nikki Sharpley (:nikkis) (she/her)]

:emilio, I think with your patch, D224201, we can add back z-indices to the sidebar box and content area to resolve Bug 1923043. Setting this as blocking for now. Thoughts?
Thanks for the patch!

Comment 10

[Emilio Cobos Álvarez (:emilio)]

I agree, but it'd be great if somebody else could deal with this because I'm a bit swamped with other work.

Dao confirmed this approach was fine, and it was green on try, modulo some failures in browser/base/content/test/performance/browser_urlbar_keyed_search.js that I haven't had time to look into. I'll push it to try without other local patches to confirm

Comment 11

[Emilio Cobos Álvarez (:emilio)]

See above. https://treeherder.mozilla.org/jobs?repo=try&revision=0ee813f4c58c3cd7ca14db0dbe45a31be49516bc is the try link

Comment 12

[Nikki Sharpley (:nikkis) (she/her)]

:emilio, I'm happy to commandeer your patch and handle the follow up work. Thank you again!

Comment 13

[Emilio Cobos Álvarez (:emilio)]

That's awesome, thanks!