Closed Bug 1351867 Opened 3 years ago Closed 3 years ago

Show search matches in a tooltip pointing at buttons if the matched text will be revealed when clicking on the button

Categories

(Firefox :: Preferences, defect, P1)

52 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
firefox55 --- fixed
firefox56 --- verified

People

(Reporter: jaws, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference] )

Attachments

(3 files, 3 obsolete files)

Attached image Overview
See attached screenshot.
Attachment #8852679 - Attachment description: Page-Shot-2017-3-29 5 3 Enter.png → Overview
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
Attachment #8853695 - Flags: review?(jaws) → review-
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
Attachment #8853695 - Flags: review?(mconley) → review-
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;
}
Attachment #8853695 - Flags: review?(jaws) → review-
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.
Attachment #8853695 - Flags: review?(mconley)
Attachment #8853695 - Flags: review?(jaws)
Blocks: 1357285
Whiteboard: [photon-preference]
Target Milestone: --- → Firefox 55
QA Contact: hani.yacoub
Flags: qe-verify+
Blocks: 1357130
No longer blocks: 1357285
Hi Manotej,

Are you still active to work on this bug?

Thanks.
Flags: needinfo?(manotejmeka)
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!
Assignee: manotejmeka → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(manotejmeka)
Blocks: 1357285
No longer blocks: 1357130
Priority: -- → P2
Target Milestone: Firefox 55 → Firefox 56
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. :)
Flags: needinfo?(rchien)
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Flags: needinfo?(rchien)
Priority: P2 → P1
Blocks: 1357130
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?
Attachment #8866310 - Flags: review?(mconley) → review-
(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?
Depends on: 1363730
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.
Attachment #8853695 - Attachment is obsolete: true
Attached patch change.diff (obsolete) — Splinter Review
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?
Attachment #8866310 - Flags: review?(mconley) → review-
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.
Attachment #8867148 - Attachment is obsolete: true
No longer depends on: 1363730
Attachment #8867166 - Attachment is obsolete: true
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?
Attachment #8866310 - Flags: review?(mconley) → review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f94466610cd0
Show search tooltip on top of button r=mconley
Depends on: 1366633
Blocks: 1368890
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.