If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Create "arrow" tooltip style for HTML tooltips

VERIFIED FIXED in Firefox 49

Status

()

Firefox
Developer Tools: Framework
P1
enhancement
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected, firefox49 verified)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Updated

a year ago
Blocks: 1259121
Severity: normal → enhancement
Depends on: 1259834
Whiteboard: [btpp-fix-later] [devtools-html]
(Assignee)

Comment 1

a year ago
Note: the position of the arrow should remain correct in RTL setups (see https://bugzilla.mozilla.org/show_bug.cgi?id=1178145 & https://bug1178145.bmoattachments.org/attachment.cgi?id=8627067)
Flags: qe-verify+
Priority: -- → P1
QA Contact: alexandra.lucinet
(Assignee)

Comment 2

a year ago
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)
(Assignee)

Updated

a year ago
Blocks: 1204914
(Assignee)

Updated

a year ago
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Iteration: --- → 49.2 - May 23
(Assignee)

Comment 4

a year ago
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)
(Assignee)

Comment 5

a year ago
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/
(Assignee)

Comment 6

a year ago
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/
(Assignee)

Comment 7

a year ago
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/
(Assignee)

Comment 8

a year ago
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.
(Assignee)

Comment 9

a year ago
try build mentioned above: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51eee685a85c
Blocks: 1266448
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+
(Assignee)

Updated

a year ago
Attachment #8752839 - Flags: feedback?(bgrinstead)
(Assignee)

Comment 12

a year ago
(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
(Assignee)

Comment 14

a year ago
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)
(Assignee)

Comment 15

a year ago
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/
(Assignee)

Comment 16

a year ago
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/
(Assignee)

Comment 17

a year ago
(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.
(Assignee)

Updated

a year ago
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)
(Assignee)

Comment 19

a year ago
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.
(Assignee)

Comment 20

a year ago
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
(Assignee)

Comment 21

a year ago
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/
(Assignee)

Comment 22

a year ago
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/
(Assignee)

Updated

a year ago
Blocks: 1275408
(Assignee)

Comment 23

a year ago
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.
(Assignee)

Comment 24

a year ago
Try https://treeherder.mozilla.org/#/jobs?repo=try&revision=09bfb15b3305

Comment 25

a year ago
https://hg.mozilla.org/integration/fx-team/rev/f8d7e7040b88
https://hg.mozilla.org/integration/fx-team/rev/5a9b084f41a5
https://hg.mozilla.org/integration/fx-team/rev/b060a42710df
Iteration: 49.2 - May 23 → 49.3 - Jun 6

Comment 26

a year 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: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Comment 27

a year 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+
You need to log in before you can comment on or make changes to this bug.