Closed Bug 1728368 Opened 2 months ago Closed 2 months ago

Findbar close button escapes findbar-close-container on macOS

Categories

(Toolkit :: Find Toolbar, task)

task

Tracking

()

VERIFIED FIXED
93 Branch
Tracking Status
firefox-esr78 --- wontfix
firefox-esr91 93+ verified
firefox91 --- wontfix
firefox92 + verified
firefox93 + verified
firefox94 --- verified

People

(Reporter: dao, Assigned: dao)

References

(Regressed 1 open bug)

Details

Attachments

(1 file)

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.

Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f6e3c384468a
Simplify findbar overflow behavior. r=emalysz
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 93 Branch
Blocks: 1728510

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

Flags: needinfo?(dao+bmo)

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: -
Flags: needinfo?(dao+bmo)
Attachment #9238729 - Flags: approval-mozilla-esr91?
Attachment #9238729 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Blocks: 1728591

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

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

Attachment #9238729 - Flags: approval-mozilla-beta? → approval-mozilla-release?
QA Whiteboard: [qa-triaged]

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.

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.
Flags: needinfo?(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.

Flags: needinfo?(ryanvm)

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.

Flags: needinfo?(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.

Flags: qe-verify+

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

Approved for 92.0.1 and 91.2esr.

Attachment #9238729 - Flags: approval-mozilla-release?
Attachment #9238729 - Flags: approval-mozilla-release+
Attachment #9238729 - Flags: approval-mozilla-esr91?
Attachment #9238729 - Flags: approval-mozilla-esr91+

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.

Duplicate of this bug: 1730142
Duplicate of this bug: 1730142
Duplicate of this bug: 1730200
Duplicate of this bug: 1730393
Regressions: 1733660
You need to log in before you can comment on or make changes to this bug.