Closed Bug 1377845 Opened 4 years ago Closed 4 years ago

Search tooltip is displayed in the wrong position within subdialogs

Categories

(Firefox :: Preferences, defect, P1)

Product:
Firefox
Component:
Preferences
Version:
56 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- verified

People

(Reporter: hani.yacoub, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(4 files)

Wrong tooltip.png
50.09 KB, image/png
Details
wrong-menuitem-label-margin.png
109.18 KB, image/png
Details
Bug 1377845 - Fix wrong tooltip position when menulist and button appear in grid
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1377845 - Fix TypeError: frame is undefined when resizing sub-dialog window
59 bytes, text/x-review-board-request
scottwu
: review+
MattN
: review+
Details
Attached image Wrong tooltip.png — 
[Affected versions]: 
Nightly 56.0a1

[Affected platforms]:
Platforms: Windows 10 x 64, Ubuntu 16.10 and Mac OS X 10.12

[Steps to reproduce]:
1. Go to "about:preferences" and search for "ff"
2. Click on "Choose..." button.
3. Observe the popup.

[Expected result]:
Tooltip should not be displayed.

[Actual result]:
Tooltip is displayed in a popup when searching for certain keywords.
Blocks: 1357285
status-firefox54: --- → unaffected
status-firefox55: --- → unaffected
status-firefox56: --- → affected
Whiteboard: [photon-preference]
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: -- → P1
Whiteboard: [photon-preference] → [photon-preference][triage]
Target Milestone: --- → Firefox 56
* Fixed TypeError: frame is undefined when resizing sub-dialog window.
* Using vbox rather than hbox or box surrounding menulist element to ensure menulist itself is able to fill available space.
* Fixed the margin for xul:label.menu-iconic-highlightable-text. see attachment.
Hey Jared,

Can you review this patch kindly? Note that this patch has fixed a couple of regressions which mentioned on comment 1. Thanks
Flags: needinfo?(jaws)
Update patch for fixing root cause of this issue.

Appending a span.search-tooltip element directly on a XUL's row element (part of grid) will cause the layout issue. The search tooltip element will be unable to take out of document flow as expected and then tooltip itself will be placed at the end of row like attachment 8882988 [details].

To fix this issue, we can wrap an extra box element (hbox/vbox/box) around all menulist and tooltip button (for those buttons which are able to open sub-dialogs) to ensure that they are inside a box element.
QA Contact: hani.yacoub
Whiteboard: [photon-preference][triage] → [photon-preference]
Attachment #8883227 - Flags: review?(jaws) → review?(MattN+bmo)
_onResize was just changed in bug 1340987 which was reviewed by MattN, so forwarding review to him.
Flags: needinfo?(jaws)
Whiteboard: [photon-preference] → [photon-preference][triage]
this is under fixing, remove triage tag
Whiteboard: [photon-preference][triage] → [photon-preference]
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> _onResize was just changed in bug 1340987 which was reviewed by MattN, so
> forwarding review to him.

…except I think that the _onResize change is fixing a separate bug and should have been a separate commit… I don't know enough about the overlays to understand whether it's really necessary to change natural markup by adding boxes around things…

Ricky, please don't combine fixes for multiple very different issues in one commit as it makes it harder to review like now where neither Jared or I can easily review the whole commit. Please split it into one commit for each bullet in comment 1.
Summary: Wrong Tooltip is displayed in a popup when searching for certain keywords → Search tooltip is displayed in the wrong position within subdialogs
Comment on attachment 8883227 [details]
Bug 1377845 - Fix wrong tooltip position when menulist and button appear in grid

https://reviewboard.mozilla.org/r/154160/#review160288

Please ask scottwu (and I) for review on the subdialogs.js change after splitting.
Attachment #8883227 - Flags: review?(MattN+bmo)
Sound great! Patch has been split into two commits for reviewing.

Jared, can you review the XUL layout changes in sub-dialogs?

Scout, can you take a look for resizing regression in subdialogs.js? It's just one line patch for fixing `TypeError: frame is undefined` when resizing sub-dialog window.

Thanks
Comment on attachment 8884684 [details]
Bug 1377845 - Fix TypeError: frame is undefined when resizing sub-dialog window

https://reviewboard.mozilla.org/r/155550/#review160554

::: commit-message-21fbd:1
(Diff revision 1)
> +Bug 1377845 - Fix TypeError: frame is undefined when resizing sub-dialog window r?scottwu

MattN should also take a look.

::: browser/components/preferences/in-content-new/subdialogs.js:331
(Diff revision 1)
>      }));
>      this._overlay.style.visibility = "visible";
>      this._overlay.style.opacity = ""; // XXX: focus hack continued from _onContentLoaded
>  
>      if (this._box.getAttribute("resizable") == "true") {
> +      this._onResize = this._onResize.bind(this);

We should also fix the issue in old preferences: http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/browser/components/preferences/in-content/subdialogs.js#331
Attachment #8884684 - Flags: review?(scwwu) → review+
Comment on attachment 8884684 [details]
Bug 1377845 - Fix TypeError: frame is undefined when resizing sub-dialog window

https://reviewboard.mozilla.org/r/155550/#review160754

Thanks
Attachment #8884684 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8883227 [details]
Bug 1377845 - Fix wrong tooltip position when menulist and button appear in grid

https://reviewboard.mozilla.org/r/154160/#review161002

I would rather not have to introduce new elements to wrap existing ones. It seems like this will be a slippery slope. I have put a patch in bug 1379208 which might give us an alternate path forward where we don't need to create new DOM elements.
Attachment #8883227 - Flags: review?(jaws) → review-
Comment on attachment 8883227 [details]
Bug 1377845 - Fix wrong tooltip position when menulist and button appear in grid

https://reviewboard.mozilla.org/r/154160/#review161292

::: browser/components/preferences/fonts.xul:100
(Diff revision 7)
>  
>            <row align="center">
>              <hbox align="center" pack="end">
>                <label accesskey="&proportional.accesskey;" control="defaultFontType">&proportional.label;</label>
>              </hbox>
> +            <hbox>

Can you please add a comment next to each one of these boxes that you're introducing stating that these are here to get search tooltips positioned correctly?

/* This <hbox> is needed to position search tooltips correctly. */
Attachment #8883227 - Flags: review?(jaws) → review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/46a7174bf44d
Fix wrong tooltip position when menulist and button appear in grid r=jaws
https://hg.mozilla.org/integration/autoland/rev/a92d98b28d10
Fix TypeError: frame is undefined when resizing sub-dialog window r=MattN,scottwu
I can reproduce this issue in Nightly 56.0a1 (2017-07-03) in 64-bit Linux

I can verify that this issue is fixed in latest Nightly 56.0a1

Build ID 	20170714100217
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [bugday-20170712]
I have reproduced this bug with Nightly 56.0a1 (2017-07-03) on Windows 8.1 , 64 Bit ! 

This bug's fix is Verified with latest Nightly 56.0a1 !

Build   ID    20170714030205
User Agent    Mozilla/5.0 (Windows NT 6.3; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0

[bugday-20170712]
As this bug is verified in both linux (comment 30) and windows (comment31), I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
You need to log in before you can comment on or make changes to this bug.