Create "arrow" tooltip style for HTML tooltips

VERIFIED FIXED in Firefox 49

Status

P1
enhancement
VERIFIED FIXED
3 years ago
5 months ago

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 49
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected, firefox49 verified)

Details

(Whiteboard: [btpp-fix-later] [devtools-html])

Attachments

(5 attachments)

XUL panel support several display types. Devtools only use the default one or arrow.

The goal of this bug is to implement the arrow type tooltip based on the default HTML tooltip implemented in Bug 1259834.

The initial style should be similar to the current XUL arrow tooltip style.
The arrow should always point to the anchor element.
Blocks: 1259121
Severity: normal → enhancement
Depends on: 1259834
Whiteboard: [btpp-fix-later] [devtools-html]

Updated

3 years ago
Flags: qe-verify+
Priority: -- → P1
QA Contact: alexandra.lucinet
Helen: I think you have mockups for new tooltips designs? 
Could you share them here so that we can have an idea of the gap between the current "arrow" XUL panel's style and the new ones?
Flags: needinfo?(hholmes)
Created attachment 8749637 [details]
popup-spec-sheet.png

Here's the spec—the bug where this info was originally posted was Bug 1258365.
Flags: needinfo?(hholmes)
Blocks: 1204914
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED

Updated

3 years ago
Iteration: --- → 49.2 - May 23
Created attachment 8752837 [details]
MozReview Request: Bug 1267401 - part1: Rename HTMLTooltip properties for backward comp with Tooltip;r=bgrins

Rename document -> doc for consistency and parent to panel
for test compatibility.

Review commit: https://reviewboard.mozilla.org/r/52812/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52812/
Attachment #8752837 - Flags: review?(bgrinstead)
Attachment #8752838 - Flags: review?(bgrinstead)
Created attachment 8752838 [details]
MozReview Request: Bug 1267401 - part2: include common.css in html tooltip tests;r=bgrins

Review commit: https://reviewboard.mozilla.org/r/52814/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52814/
Created attachment 8752839 [details]
MozReview Request: Bug 1267401 - part3: Create arrow style for HTML tooltips;r=bgrins

Review commit: https://reviewboard.mozilla.org/r/52816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52816/
Comment on attachment 8752839 [details]
MozReview Request: Bug 1267401 - part3: Create arrow style for HTML tooltips;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52816/diff/1-2/
Created attachment 8752923 [details]
html-tooltips-demo.gif

I initially implemented the new tooltip design from Helen. Then I had both the old and new designs supported as 2 different tooltip styles. 

But I think we should first focus on landing these changes and then we can think about using the new style. 

Except for the limitations linked to the HTMLTooltip implementation (the tooltip has to fit in the toolbox), I tried to remain as close as possible to the existing UI.

There is an ongoing try build with the patches from this Bug and from Bug 1266448. Feel free to retrieve the builds and to try out the image preview tooltips in the markup view.
Comment on attachment 8752837 [details]
MozReview Request: Bug 1267401 - part1: Rename HTMLTooltip properties for backward comp with Tooltip;r=bgrins

https://reviewboard.mozilla.org/r/52812/#review50158
Attachment #8752837 - Flags: review?(bgrinstead) → review+
Comment on attachment 8752838 [details]
MozReview Request: Bug 1267401 - part2: include common.css in html tooltip tests;r=bgrins

https://reviewboard.mozilla.org/r/52814/#review50162

Seems fine - why is this needed?
Attachment #8752838 - Flags: review?(bgrinstead) → review+
Attachment #8752839 - Flags: feedback?(bgrinstead)
(In reply to (Unavailable until May 23) Brian Grinstead [:bgrins] from comment #11)
> Comment on attachment 8752838 [details]
> MozReview Request: Bug 1267401 - part2: include common.css in html tooltip
> tests;r=bgrins
> 
> https://reviewboard.mozilla.org/r/52814/#review50162
> 
> Seems fine - why is this needed?

I could have merged this with the part3. I started using a classname in order to switch the visibility of the tooltip. Originally this was because I was experimenting with tooltips relying on display:flex rather than display:block and it was more convenient to do this in CSS.
https://reviewboard.mozilla.org/r/52816/#review51262

I'm getting this error when running the tests locally: 21 INFO TEST-UNEXPECTED-FAIL | devtools/client/shared/test/browser_html_tooltip_arrow-02.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/shared/test/helper_html_tooltip.js:57 - TypeError: document is undefined
Comment on attachment 8752837 [details]
MozReview Request: Bug 1267401 - part1: Rename HTMLTooltip properties for backward comp with Tooltip;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52812/diff/1-2/
Attachment #8752839 - Flags: feedback?(bgrinstead)
Comment on attachment 8752838 [details]
MozReview Request: Bug 1267401 - part2: include common.css in html tooltip tests;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52814/diff/1-2/
Comment on attachment 8752839 [details]
MozReview Request: Bug 1267401 - part3: Create arrow style for HTML tooltips;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52816/diff/2-3/
(In reply to Brian Grinstead [:bgrins] from comment #13)
> https://reviewboard.mozilla.org/r/52816/#review51262
> 
> I'm getting this error when running the tests locally: 21 INFO
> TEST-UNEXPECTED-FAIL |
> devtools/client/shared/test/browser_html_tooltip_arrow-02.js | Uncaught
> exception - at
> chrome://mochitests/content/browser/devtools/client/shared/test/
> helper_html_tooltip.js:57 - TypeError: document is undefined

Indeed, needed rebasing and fixing. Updated the review requests accordingly.
Attachment #8752839 - Flags: feedback?(bgrinstead)
Attachment #8752839 - Flags: review+
Comment on attachment 8752839 [details]
MozReview Request: Bug 1267401 - part3: Create arrow style for HTML tooltips;r=bgrins

https://reviewboard.mozilla.org/r/52816/#review51514

Nice job, this looks good

::: devtools/client/shared/test/browser_html_tooltip_arrow-01.js:31
(Diff revision 3)
> +  <window class="theme-light"
> +          xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +          xmlns:html="http://www.w3.org/1999/xhtml"
> +          title="Tooltip test">
> +    <vbox flex="1" style="position: relative">
> +      ${getAnchor("top: 0; left: 0;")}

I noticed the arrow doesn't quite make it to the anchor when it's all the way to the right or left.  I guess that's the blocked by viewport case.

It's not a big deal - we could work on alternate styling for that case later on if needed
Attachment #8752839 - Flags: feedback?(bgrinstead)
https://reviewboard.mozilla.org/r/52816/#review51514

Thanks for the review, will land after resubmitting to try!

> I noticed the arrow doesn't quite make it to the anchor when it's all the way to the right or left.  I guess that's the blocked by viewport case.
> 
> It's not a big deal - we could work on alternate styling for that case later on if needed

I think this limitation is acceptable for now.

As you said, we need to use a different arrow style if we want to match anchors very close to the edges of the viewport.
I will open a follow up for this.
Comment on attachment 8752837 [details]
MozReview Request: Bug 1267401 - part1: Rename HTMLTooltip properties for backward comp with Tooltip;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52812/diff/2-3/
Attachment #8752839 - Attachment description: MozReview Request: Bug 1267401 - part3: Create arrow style for HTML tooltips;f=bgrins → MozReview Request: Bug 1267401 - part3: Create arrow style for HTML tooltips;r=bgrins
Comment on attachment 8752838 [details]
MozReview Request: Bug 1267401 - part2: include common.css in html tooltip tests;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52814/diff/2-3/
Comment on attachment 8752839 [details]
MozReview Request: Bug 1267401 - part3: Create arrow style for HTML tooltips;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52816/diff/3-4/
Blocks: 1275408
https://reviewboard.mozilla.org/r/52816/#review51514

> I think this limitation is acceptable for now.
> 
> As you said, we need to use a different arrow style if we want to match anchors very close to the edges of the viewport.
> I will open a follow up for this.

Opened Bug 1275408, dropping the issue here.

Updated

3 years ago
Iteration: 49.2 - May 23 → 49.3 - Jun 6

Comment 26

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f8d7e7040b88
https://hg.mozilla.org/mozilla-central/rev/5a9b084f41a5
https://hg.mozilla.org/mozilla-central/rev/b060a42710df
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Comment 27

3 years ago
The tooltip is properly displayed with latest Nightly 49.0a1 en-US and ar (RTL) builds, under Windows 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.10.5.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-dthtml]
status-firefox49: fixed → verified
Flags: qe-verify+

Updated

5 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.