1728368 - Findbar close button escapes findbar-close-container on macOS

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, task)

Description

[Dão Gottwald [:dao]]

I looked at bug 1724194's patch and although I haven't tested it, it looks like this isn't going to work consistently across OSes because of this macOS-specific code: https://searchfox.org/mozilla-central/rev/ad2ffab089e4e0c0fe99a1a046ab2b1c45546bdb/toolkit/content/widgets/findbar.js#103-108

There are other issues with the patch:

• Elements created by toolkit widgets should use class rather than id since a window can have multiple findbar elements.
• The browser.inc.css changes like the following one don't seem to make sense:

+.browserContainer > findbar > #findbar-close-container,
 .browserContainer > findbar {
   background-color: var(--toolbar-non-lwt-bgcolor);
   color: var(--toolbar-non-lwt-textcolor);
   text-shadow: none;
 }

#findbar-close-container is already a child of findbar, so it seems to make no sense to apply the same background again.

Anyway, I think we can just get rid of #findbar-close-container and handle overflow in a simpler way.

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1728368 - Simplify findbar overflow behavior. r=emalysz

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6e3c384468a
Simplify findbar overflow behavior. r=emalysz

Comment 3

[Cristian Tuns]

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

Comment 4

[Alice0775 White]

::dao,
Would you consider uplifting this to 92 to fix bug 1728510?

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 9238729 [details]
Bug 1728368 - Simplify findbar overflow behavior. r=emalysz

Beta/Release Uplift Approval Request

• User impact if declined: Bug 1728510 (caused by bug 1724194)
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: We need to verify with the steps from both bug 1728510 and bug 1724194
• List of other uplifts needed: -
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is mostly a reversal of bug 1724194, with the addition of a simpler, one-line fix to that bug
• String changes made/needed: -

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Bug 1724194 was uplifted to ESR
• User impact if declined: Bug 1728510 (caused by bug 1724194)
• Fix Landed on Version: Nightly 93
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is mostly a reversal of bug 1724194, with the addition of a simpler, one-line fix to that bug
• String or UUID changes made by this patch: -

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9238729 [details]
Bug 1728368 - Simplify findbar overflow behavior. r=emalysz

Fx92 is on release now, moving the request over accordingly.

Comment 7

[Daniel Bodea [:danibodea]]

As requested in the uplift request, both bug 1724194 and bug 1728510 are now properly fixed in Nightly v93.0a1 form 2021-09-03, however, neither of those bugs reproduce on Mac OS, but only on Windows and Linux systems.

While assuming it is intended (or rather enough) to verify only Windows 10 and Ubuntu 20, I will set firefox93 flag as verified and the others as affected.

Waiting for uplifts to verify on the other channels.

Comment 8

[Daniel Bodea [:danibodea]]

May I ask why you've set these versions as unaffected?
I can still reproduce the disappearance of the 'x' button in certain situations.

STR:

1. Open browser.
2. Make the window be in a "restore down" mode (not maximized)
3. Open the Find bar (CTRL+F).
4. Catch a side frame of the window and make it very narrow.
   Actual: The x button disappears (is pushed out of the find bar).
   Expected: The x button remains on top of the other controls.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

The regressing bug never landed on the 2 releases I marked as unaffected. If you're reproducing the issue on those as well, we probably need to revisit the regression range on this bug.

Comment 10

[Daniel Bodea [:danibodea]]

This bug apparently addresses the issues in bug 1728510 and bug 1724194:

• Bug 1728510:
  ** is the fact that the find bar's 'X' button is pushed out of the screen when opening the sidebar, which happened even if the browser width was wide or even maximized.
  ** is regressed by the fix of bug 1724194 (verified).

• Bug 1724194:
  ** is the fact that the find bar's 'X' button is pushed out of the screen when the longer message "Reached end of page, continued from top" gets displayed, which only happened when the width of the page is narrow enough.
  ** the steps in this bug 1724194 could be simplified with my steps in comment 8 (more clear and easy to verify).
  ** it was initially partially fixed as the 'X' button would not spill out of the window when the window is wide enough, but the 'X' button would still spill out when narrowing the window down (or using the steps with the long message displayed). This fix regressed bug 1728510.
  ** it is said to have been regressed by bug 1690334 which landed in Fx89, but ACTUALLY, I don't agree. It seems to me that this is not even a regression at all because it reproduces in builds even before proton (like v50.0a1).

• The fix provided in THIS bug, has completed the fix of bug 1724194 AND also fixed bug 1728510, as the 'X' button is now always on top now.

• It (STR in comment 8) is still reproduced in Release v91.0.1 and ESR 78.14.0esr, but it does not reproduce anymore in Release 92.0 OR ESR 91.1.0esr, however, if I don't see any references of an uplift to these channels, only the request!

Could you help sort this out?
Feel free to contact me again if more info is needed.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

OK, it looks like there was another pre-existing issue there which was made worse by bug 1724194. Glad to hear that things are better with this patch.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9238729 [details]
Bug 1728368 - Simplify findbar overflow behavior. r=emalysz

Approved for 92.0.1 and 91.2esr.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/5da191b2f7d6

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/0429734423a9

Comment 15

[Daniel Bodea [:danibodea]]

Both the issues in bug 1728510 and bug 1724194 were properly verified in:

• Release v92.0.1 from Treeherder, build ID 20210909153504.
• ESR v91.2.0esr from Treeherder, build ID 20210909152805.
• Nightly v94.0a1 from 2021-09-09, build ID 20210909212746.

Uplift verification complete.