Closed Bug 1590408 Opened 4 months ago Closed 4 months ago

HTMLTooltip / MenuButton is off when the anchor is next to the screen edge

Categories

(DevTools :: Shared Components, defect, P2)

defect

Tracking

(firefox-esr68 unaffected, firefox70 unaffected, firefox71+ verified, firefox72 verified)

VERIFIED FIXED
Firefox 72
Tracking Status
firefox-esr68 --- unaffected
firefox70 --- unaffected
firefox71 + verified
firefox72 --- verified

People

(Reporter: nchevobbe, Assigned: jdescottes)

References

(Regression)

Details

(Keywords: regression)

Attachments

(5 files, 1 obsolete file)

Attached image Group.png

Steps to reproduce

  1. Open the console
  2. Hit <kbd>Ctrl</kbd>+<kbd>+</kbd> (or <kbd>Cmd</kbd> on mac) to zoom-in
  3. Make sure the firefox right border
  4. Click on the console settings button

Expected results

The menu opens and its arrow point to the button

Actual results

The menu appears on the left of the button, looking mis-aligned


I also got a report from someone saying they didn't zoomed in. May depend on screen pixel ratio

Note this also reproduces with the DevTools main menu (meatball menu)

I'm raising the priority of this, I can confirm it happens without zooming (on a regular non-retina screen). Given the placement of the Console settings menu button, it's likely to impact any user with devtools in fullscreen/fullwidth.

Apparently doesn't happen on Linux? Would be interesting to know if it impacts windows or not.

Blocks: 1523868
Priority: P3 → P2
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Attached image image.png

I think the regression is coming from the fact that the arrow was shifted to be further away from the tooltip's edge. On the screenshot, you can see how it is now vs how it was before.

We take this offset into consideration to calculate the position of all the elements. With the previous version (Fx 70), the meatball menu was always correctly displayed.
It's very easy to upset the HTML tooltip positioning code, and here I guess it only regressed for the DOORHANGER type which is not as tested as the old ARROW type.

Keywords: regression
Regressed by: 1580936
Regressed by: 1552146
No longer regressed by: 1580936

(I need some more time to find the right regressing bug, sorry about the noise)

No longer regressed by: 1552146

I confirm this is a regression from Bug 1552146. This patch introduces a 1px offset in the middle of the HTML tooltip positioning only on non-hidpi screens.

I was wrong in comment #3, this has nothing to do with the discrepancy between the arrow offset in JS and the actual offset.

Regressed by: 1552146

Some more details about this. Regression from the patch https://hg.mozilla.org/mozilla-central/rev/84708a4f040d
Especially here, it's because we are updating the left position of the tooltip at the very end of the calculation. The HTMLTooltip calculates its position so that it never crosses the edge of the screen, because trying to draw a XUL panel outside of the screen can lead to weird behaviors (as this bug attests).

So if we want to "shift" the position of the tooltip, we need to do it before we check for constraints:

Adding pixels after those steps can lead the panel to be displayed off screen, as it does here. I think that if the calculations are "off", there is probably a flaw in the calculation.

For instance I think the tooltip is displayed too "high" because the arrow is created by rotating a non-square element, so the tip of the arrow is not on the edge of its container, there's a gap. Using highlighters is a good way to investigate such issues (but it's still a pain...). If we add negative margins to acknowledge that in .tooltip-top .tooltip-bottom:

 .tooltip-top .tooltip-arrow {
   margin-top: -1px;
+  margin-bottom: -2px;
 }
 
 .tooltip-bottom .tooltip-arrow {
+  margin-top: -2px;
   margin-bottom: -1px;
 }

then the calculation will be correct because the size of the arrow element will match the "visual" size of the element.

For the 1px horizontal offset, I didn't manage to find where the discrepancy was coming from though. Maybe a border not taken into consideration?

I think we should first provide a simple fix here to address the regression and uplift this to 71. Maybe remove the code that touches the left position for now until we have a better solution? Mike since you wrote the original patch, do you have a preference/suggestion?

Flags: needinfo?(mratcliffe)

I think that a simple fix and uplifting this to 71 makes sense.

Flags: needinfo?(mratcliffe)

Will add a test in a subsequent changeset

Attachment #9108037 - Attachment is obsolete: true

The positioning of the arrow was correct, but due to the way the arrow was rotated, the visual center of the arrow did not match the center of the arrow box
Backing out the JS change here, CSS patch later in the queue

Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/40b9a55b447d
Backed out changeset 84708a4f040d from Bug 1552146 r=miker
https://hg.mozilla.org/integration/autoland/rev/f1293ec6b6da
Add test to check doorhanger tooltip display on screen edge r=miker
https://hg.mozilla.org/integration/autoland/rev/5b26b747f3bd
Fix CSS for doorhanger HTMLTooltip to align the center of the arrow with the center of the anchor r=miker

Comment on attachment 9108095 [details]
Bug 1590408 - Fix CSS for doorhanger HTMLTooltip to align the center of the arrow with the center of the anchor

Beta/Release Uplift Approval Request

  • User impact if declined: DevTools main menu can appear at the wrong position (as in: middle of the screen, not just a small offset) when devtools are near the edge of the screen
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): All three patches should be safe to uplift:
  • the first is a backout is removing a JS attempt at fixing a visual nit (small 1px positioning offset)
  • the second is a test to check the regression introduced by the backed out patch
  • the third is a CSS fix that achieves the same thing as the backed out patch, without the side effects
  • String changes made/needed:
Attachment #9108095 - Flags: approval-mozilla-beta?
Attachment #9108055 - Flags: approval-mozilla-beta?
Attachment #9108093 - Flags: approval-mozilla-beta?
Summary: HTMLTooltip / MenuButton is off when the anchor is next to the screen edge (and document is zoomed-in) → HTMLTooltip / MenuButton is off when the anchor is next to the screen edge
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 72
Flags: qe-verify+

Comment on attachment 9108095 [details]
Bug 1590408 - Fix CSS for doorhanger HTMLTooltip to align the center of the arrow with the center of the anchor

Fix for a 71 visual regression in devtools, patch covered by tests, uplifts approved for 71 beta 10, thanks.

Attachment #9108095 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9108055 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9108093 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Confirmed issue with 72.0a1 (2019-10-22) on Windows 10.
Fix verified with 72.0a1 (2019-11-13) on Windows 10.

Fix would be verified as well for 71.0b10.

However, before updating the flags, need to confirm a scenario that might have been overlooked or if it is another issue altogether.
With a dual monitor setup:

  • browser on the right monitor docked to the right - > fixed and all ok;
  • browser on the left monitor docked to right -> still has the issue.

Do we need to file a separate bug for the mentioned issue?

Flags: needinfo?(jdescottes)

This bug is about fixing the regression from Bug 1552146.
If the STRs above already occur before Bug 1552146 (eg in 70), then we should file another bug.
Note that we have Bug 1596120 that also reports an issue with Dual Screen + DevTools tooltips, although your issue is probably worth filing another bug.

Flags: needinfo?(jdescottes)

Indeed, bug 1596120 appears to match my findings. Left a note in it.
Marking this bug as verified since the patch works as intended.
Thanks again!

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.