See attached screenshot.

Comment on attachment 8853695 [details] Bug 1351867 - Implementing Tooltip/Bubble for about:preferences search feature. https://reviewboard.mozilla.org/r/125774/#review131124 ::: browser/components/preferences/in-content/findInPage.js:11 (Diff revision 5) > > var gSearchResultsPane = { > findSelection: null, > searchResultsCategory: null, > searchInput: null, > + listSearchBubbles: [], Please run eslint on your code. There shouldn't be tabs mixed with spaces. ::: browser/components/preferences/in-content/findInPage.js:32 (Diff revision 5) > if (event.type === "command") { > this.searchFunction(event); > } else if (event.type === "focus") { > this.initializeCategories(); > + } else if (event.type === "mouseenter" && > + event.target.className.indexOf("search-bubble") > -1) { You should use event.target.classList.contains. ::: browser/components/preferences/in-content/findInPage.js:40 (Diff revision 5) > + > + // Note: When searching for Google, Yahoo then tooltip is off ask for help. > + let parent = event.target; > + let child = parent.firstChild; > + let rect = parent.getBoundingClientRect(); > + let heightOffSet = rect.height * 3; Why does the height need to be tripled here? ::: browser/components/preferences/in-content/findInPage.js:43 (Diff revision 5) > + let child = parent.firstChild; > + let rect = parent.getBoundingClientRect(); > + let heightOffSet = rect.height * 3; > + > + if (child.className.indexOf("reverse") != -1) { > + child.className = "search-bubble-inner"; You should move this line up outside of the if/else because it "search-bubble-inner" is always set. And you should always be using the element.classList API instead of className. ::: browser/components/preferences/in-content/findInPage.js:44 (Diff revision 5) > + let rect = parent.getBoundingClientRect(); > + let heightOffSet = rect.height * 3; > + > + if (child.className.indexOf("reverse") != -1) { > + child.className = "search-bubble-inner"; > + parent.style.cssText = "top:" + (parseFloat(parent.style.top.slice(0, -2)) + heightOffSet).toString() + "px; left:" + parent.style.left.toString() + "; z-index:2;"; These should be setting CSS variables via element.style.setProperty API instead of the cssText API. ::: browser/components/preferences/in-content/findInPage.js:46 (Diff revision 5) > + > + if (child.className.indexOf("reverse") != -1) { > + child.className = "search-bubble-inner"; > + parent.style.cssText = "top:" + (parseFloat(parent.style.top.slice(0, -2)) + heightOffSet).toString() + "px; left:" + parent.style.left.toString() + "; z-index:2;"; > + } else { > + child.className = "search-bubble-inner reverse"; child.classList.add("reverse") ::: browser/components/preferences/in-content/findInPage.js:214 (Diff revision 5) > + [].forEach.call(document.querySelectorAll(".search-bubble"), function(child) { > + let parent = child.parentNode; > + parent.removeChild(child); > + }); let searchBubbles = Array.from(document.querySelectorAll(".search-bubble"); for (let searchBubble of searchBubbles) { searchBubble.removeEventListener("mouseenter", this); searchBubble.remove(); } ::: browser/components/preferences/in-content/findInPage.js:368 (Diff revision 5) > + searchBubble.setAttribute("class", "search-bubble"); > + > + let offSet = (rect.width / 2) - 50; > + // First element in the row and has no sibiling before it > + if (!currentNode.previousElementSibling) { > + searchBubble.style.cssText = "top:" + (rect.height + 10).toString() + "px; left:" + (offSet + 4).toString() + "px; z-index:2;"; I think you should be able to use `bottom` instead of `top`, and a negative number and then you might not need to use rect.height and can move this directly in to the CSS file? Similar thing for `left`? ::: browser/components/preferences/in-content/findInPage.js:376 (Diff revision 5) > + offSet += (rect.left - parentRect.left); > + searchBubble.style.cssText = "top:" + (rect.height + 10).toString() + "px; left:" + offSet.toString() + "px; z-index:2;"; > - } > + } > + let searchContent = document.createElement("div"); > + searchContent.setAttribute("class", "search-bubble-inner"); > + searchContent.innerHTML = searchPhrase; This cannot be innerHTML as that is a security bug. We cannot put user-supplied text through innerHTML. You must use the textContent API instead. ::: browser/themes/shared/incontentprefs/preferences.inc.css:588 (Diff revision 5) > padding-inline-start: 20px; > } > + > +/* Container for the elements that make up the search bubble. */ > +.search-bubble { > + background: -moz-linear-gradient(rgba(255, 248, 172, 0.9), Please use `linear-gradient` instead of `-moz-linear-gradient`. Also, this should be `background-image` instead of `background`. ::: browser/themes/shared/incontentprefs/preferences.inc.css:594 (Diff revision 5) > + rgba(255, 243, 128, 0.9)); > + left: -30px; > + position: absolute; > + text-align: center; > + width: 100px; > + top: -1000px; /* Minor hack: position off-screen by default. */ Why do we need to position this off-screen? ::: browser/themes/shared/incontentprefs/preferences.inc.css:605 (Diff revision 5) > +/* Provides the border around the bubble (has to be behind ::after). */ > +.search-bubble-inner::before { > + border: 1px solid rgb(220, 198, 72); I don't think ::before is necessary just to draw a border? Can you move this border to the .search-bubble? ::: browser/themes/shared/incontentprefs/preferences.inc.css:620 (Diff revision 5) > + z-index: -2; > +} > + > +/* Provides the arrow which points at the anchor element. */ > +.search-bubble-inner::after { > + -moz-transform: rotate(45deg); transform instead of -moz-transform. ::: browser/themes/shared/incontentprefs/preferences.inc.css:622 (Diff revision 5) > + > +/* Provides the arrow which points at the anchor element. */ > +.search-bubble-inner::after { > + -moz-transform: rotate(45deg); > + background: > + -moz-linear-gradient(-45deg, rgb(251, 255, 181), linear-gradient instead of -moz-linear-gradient. ::: browser/themes/shared/incontentprefs/preferences.inc.css:640 (Diff revision 5) > +} > + > +/* Turns the arrow direction downwards, when the bubble is placed above the > + * anchor element */ > +.search-bubble-inner.reverse::after { > + -moz-transform: rotate(-135deg); transform instead of -moz-transform

Comment on attachment 8853695 [details] Bug 1351867 - Implementing Tooltip/Bubble for about:preferences search feature. https://reviewboard.mozilla.org/r/125774/#review131508 ::: browser/components/preferences/in-content/findInPage.js:360 (Diff revision 5) > + currentNode.parentElement.style.cssText = "position:relative"; > + currentNode.style.cssText = "z-index:1"; Why is it necessary to manually set these static styles? Can we not apply an attribute to a class and let CSS do the work? ::: browser/components/preferences/in-content/findInPage.js:362 (Diff revision 5) > + let searchBubble = document.createElement("div"); > + searchBubble.setAttribute("class", "search-bubble"); A lot of work is being done in order to position this popup so that it's in the same general area as the currentNode. Can we not use the ::before or [::after](https://developer.mozilla.org/en-US/docs/Web/CSS/::after) pseudoelement instead? I think with relative positioning, we should be able to get what we want with those. Note that what you're doing here (getting the rect of a node) while in a loop, where that loop is also adding or changing styles on a page, is called "layout thrashing", and is not something we can ship. Please see https://docs.google.com/document/d/1zJaemtvZzDHcLfFH6K_CBZt8xGPIuvwyeyhjD0HRSlA/edit#heading=h.rpdmq0vz5nur

Comment on attachment 8853695 [details] Bug 1351867 - Implementing Tooltip/Bubble for about:preferences search feature. https://reviewboard.mozilla.org/r/125774/#review132074 ::: browser/components/preferences/in-content/findInPage.js:32 (Diff revisions 5 - 6) > if (event.type === "command") { > this.searchFunction(event); > } else if (event.type === "focus") { > this.initializeCategories(); > } else if (event.type === "mouseenter" && > - event.target.className.indexOf("search-bubble") > -1) { > + event.target.classList.contains("search-bubble")) { nit, please align this line with the start of the condition above it. like so: > } else if (event.type === "mouseenter" && > event.target.classList.contains("search-bubble") { ::: browser/components/preferences/in-content/findInPage.js:45 (Diff revisions 5 - 6) > - if (child.className.indexOf("reverse") != -1) { > - child.className = "search-bubble-inner"; > + if (child.classList.contains("reverse")) { > + child.classList.remove("reverse"); need to remove all tabs in here. it's hard to review and understand code flow with tabs inserted. ::: browser/components/preferences/in-content/findInPage.js:47 (Diff revisions 5 - 6) > + // above or below of the element > let heightOffSet = rect.height * 3; > > - if (child.className.indexOf("reverse") != -1) { > - child.className = "search-bubble-inner"; > - parent.style.cssText = "top:" + (parseFloat(parent.style.top.slice(0, -2)) + heightOffSet).toString() + "px; left:" + parent.style.left.toString() + "; z-index:2;"; > + if (child.classList.contains("reverse")) { > + child.classList.remove("reverse"); > + parent.style.setProperty("bottom", "-31px"); it looks like mconley's review comment about why these are hardcoded wasn't addressed. ::: browser/themes/shared/incontentprefs/preferences.inc.css:588 (Diff revisions 5 - 6) > } > > /* Provides the arrow which points at the anchor element. */ > .search-bubble-inner::after { > - -moz-transform: rotate(45deg); > + transform: rotate(45deg); > background: s/background:/background-image:/ ::: browser/themes/shared/incontentprefs/preferences.inc.css:597 (Diff revisions 5 - 6) > border: 1px solid rgb(220, 198, 72); > border-bottom-color: transparent; > border-right-color: transparent; > content: ''; > height: 12px; > left: 47px; The arrow should be on the other end if the text is RTL. Please add another section that unsets this and sets the `right` value in the case of RTL. .search-bubble-inner:-moz-locale-dir(rtl)::after { left: auto; right: 47px; }

Comment on attachment 8853695 [details] Bug 1351867 - Implementing Tooltip/Bubble for about:preferences search feature. Clearing review since the patch commit message doesn't include r?jaws or r?mconley. Please contact me if you wanted a review.

Hi Manotej, Are you still active to work on this bug? Thanks.

Hey Manotej - I'm going to unassign you from this one. A full-timer will drive this one through to a resolution. Thanks for your work!

Hi Ricky, If you have time, could you investigate this bug. I'll investigate Bug 1352481 once I have time. Let's try to investigate and do something this week. :)

See also http://kushagragour.in/lab/hint/

The visual design spec is [1] from https://bugzilla.mozilla.org/show_bug.cgi?id=1357130#c2. [1]: https://mozilla.invisionapp.com/share/ZDAGPK3AF#/screens/219340982

Unfortunately, the pure CSS tooltip solution (using after before pseudo element) doesn't work for XUL element :( Try to set `position: absolute` in pseudo element is unable to takes the element out of flow. The only way to draw tooltip relies on calculating position manually via JS. So I think Manotej's implementation is reasonable. I think the current patch is ready to ask first round review. Thanks!

Comment on attachment 8866310 [details] Bug 1351867 - Show search tooltip on top of button https://reviewboard.mozilla.org/r/137942/#review141072 ::: browser/components/preferences/in-content/findInPage.js:247 (Diff revision 1) > + // Creating tooltips for all the instances found > + for (let node of this.listSearchTooltips) { > + this.createSearchTooltip(node, query); > + } This is going to thrash layout, and is going to be a performance problem. You're querying for size / position information, and dirtying the DOM (by adding nodes) in a loop. The first query for size / position information will probably also cause synchronous style / layout flush because the DOM is likely already dirty at the time that the search starts. Have you had no luck getting help with the 50% left style calculation for pseudoelements in XUL?

(In reply to Ricky Chien [:rickychien] from comment #20) > Try to set `position: absolute` in pseudo element is unable to takes the > element out of flow. The only way to draw tooltip relies on calculating > position manually via JS. Unless we fix the XUL bug. Any success talking to tn about it?

Agree! I'm still prefer to use pure CSS solution and avoid slowing down performance. Bug 1363730 has reported for asking tnikkel about this issue. Let's wait and see.

So I've got good news, and bad news. The good news is that your code actually, at least to my testing, doesn't add a new synchronous layout flush when adding tooltips! I think this is mainly because there's already a flush occurring earlier when we call scrollTop in gotoPref. The bad news is that your code _will_ cause a flush if we ever get rid of scrollTop (which we should probably do). I found a way of avoiding the potential synchronous reflow by waiting for the next paint before doing measurement, and then setting the position of the tooltip in a requestAnimationFrame. This, unfortunately, relies on the MozAfterPaint event firing in the content area, which doesn't normally happen (not without dom.send_after_paint_to_content set to true). So I don't think my solution is shippable as is. So, having weighed reality, I think XUL has yet again beaten us. I think we're going to have to take the potential for synchronous flush here. :(

Comment on attachment 8866310 [details] Bug 1351867 - Show search tooltip on top of button https://reviewboard.mozilla.org/r/137942/#review143192 Hey - thanks, and sorry for the delay. I have a few questions - see below. ::: browser/components/preferences/in-content/findInPage.js:357 (Diff revision 2) > + > + let searchTooltip = document.createElement("span"); > + searchTooltip.setAttribute("class", "search-tooltip"); > + searchTooltip.textContent = query; > + > + currentNode.parentElement.appendChild(searchTooltip); Can you add a comment here that we're flushing layout intentionally here, because we need the up-to-date position of each of the nodes that we're putting tooltips on, and that this is the result of a XUL limitation (and then link to that bug you filed). ::: browser/components/preferences/in-content/main.xul:491 (Diff revision 2) > > <!-- Fonts and Colors --> > <groupbox id="fontsGroup" data-category="paneGeneral" hidden="true"> > <caption><label>&fontsAndColors.label;</label></caption> > > - <grid id="fontsGrid"> > + <vbox> Why are these changes to the DOM necessary? ::: browser/components/preferences/in-content/privacy.xul:309 (Diff revision 2) > > <!-- Passwords --> > <groupbox id="passwordsGroup" orient="vertical" data-category="panePrivacy" hidden="true"> > <caption><label>&formsAndPasswords.label;</label></caption> > > - <grid id="passwordGrid"> > + <vbox id="passwordRows"> Same as above - why is this needed? ::: browser/themes/shared/incontentprefs/preferences.inc.css:626 (Diff revision 2) > +.search-tooltip::before { > + position: absolute; > + content: ""; > + border: 6px solid transparent; > + border-top-color: #ffe352; > + left: calc(50% - 6px); Is this left: rule still necessary since we're programmatically setting the tooltip left?

Comment on attachment 8866310 [details] Bug 1351867 - Show search tooltip on top of button https://reviewboard.mozilla.org/r/137942/#review143192 > Why are these changes to the DOM necessary? <grid> and <column> layout will introduce a weird position behavior and tooltip would be unable to jump out the content flow. I think it's an another XUL issue but it can be workaround easily after replacing grid or column elements with vbox / hbox. > Is this left: rule still necessary since we're programmatically setting the tooltip left? This is necessary for positioning the .search-tooltip::before a.k.a the triangle part below the tooltip.

The patch has updated and supported RTL. Please check it again. Thanks!

Comment on attachment 8866310 [details] Bug 1351867 - Show search tooltip on top of button https://reviewboard.mozilla.org/r/137942/#review144776 I did a quick check, and this looks okay on my Windows machine. I didn't see anybody referring to the old IDs that you're changing either. r=me with the below changes. ::: browser/components/preferences/in-content/privacy.xul:309 (Diff revision 5) > > <!-- Passwords --> > <groupbox id="passwordsGroup" orient="vertical" data-category="panePrivacy" hidden="true"> > <caption><label>&formsAndPasswords.label;</label></caption> > > - <grid id="passwordGrid"> > + <vbox id="passwordRows"> passwordSettings? I don't know. Seems strange to keep "rows" around, especially since you removed it for Fonts too. ::: browser/components/preferences/in-content/privacy.xul:322 (Diff revision 5) > - class="accessory-button" > + class="accessory-button" > - label="&passwordExceptions.label;" > + label="&passwordExceptions.label;" > - accesskey="&passwordExceptions.accesskey;" > + accesskey="&passwordExceptions.accesskey;" > - preference="pref.privacy.disable_button.view_passwords_exceptions"/> > + preference="pref.privacy.disable_button.view_passwords_exceptions"/> > - </row> > - <row id="showPasswordRow"> > + </hbox> > + <hbox id="showPasswordRow"> showPasswordBox?

Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f94466610cd0 Show search tooltip on top of button r=mconley

Build ID: 20170709030204 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0 Verified as fixed on Firefox Nightly 56.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.