Closed Bug 1460691 Opened 2 years ago Closed 2 years ago

Tooltip on 'clear' button doesn't show up when the Browser Console is running as a top-level HTML window

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: bgrins, Assigned: bdahl)

References

Details

Attachments

(1 file)

STR:
* ./mach run --setpref devtools.browserconsole.html=true --jsconsole
* Hover the clear messages (trash can) icon in the Browser Console

Expected:
Tooltip text shows up

Actual:
No tooltip text shows up

The hover text does show up when running inside the normal toolbox, I'm assuming because of `this.frame.tooltip = "aHTMLTooltip";` in toolbox-hosts.js.
Priority: -- → P3
Product: Firefox → DevTools
Blocks: 1469341
No longer depends on: 1469341
Comment on attachment 8989558 [details]
Bug 1460691 - Support tooltips in top level non-XUL windows. +6102

https://reviewboard.mozilla.org/r/254586/#review261472

r+ on the CanvasFrame changes, but please get enndeakin or someone else to review the rest (I'm going on vacation so I don't have time to page that code in)

::: layout/generic/nsCanvasFrame.cpp:341
(Diff revision 1)
>  void
>  nsCanvasFrame::SetDefaultTooltip(Element* aTooltip)
>  {
> -  NS_WARNING("SetDefaultTooltip not implemented");
> -  return;
> +  MOZ_ASSERT(!aTooltip || aTooltip == mTooltipContent,
> +             "Default tooltip should be anonymous content tooltip.");
> +  aTooltip = mTooltipContent;

This assignment should be the other way around.
Attachment #8989558 - Flags: review?(mats) → review+
Assignee: nobody → bdahl
Comment on attachment 8989558 [details]
Bug 1460691 - Support tooltips in top level non-XUL windows. +6102

See above^
Attachment #8989558 - Flags: review+ → review?(enndeakin)
Attachment #8989558 - Flags: review?(enndeakin)
Comment on attachment 8989558 [details]
Bug 1460691 - Support tooltips in top level non-XUL windows. +6102

https://reviewboard.mozilla.org/r/254586/#review263514

::: layout/xul/nsXULTooltipListener.cpp:627
(Diff revision 2)
>        }
>      }
>    }
>  
>  #ifdef MOZ_XUL
> -  // titletips should just use the default tooltip
> +  // titletips and non-XUL documents should just use the default tooltip

The tooltip listener doesn't have any specific checks for XUL elements (most specifically in GetTooltipFor), so if the various attributes it retrieves are not on XUL elements, it would check attributes that shouldn't have any meaning.

Is that desired?
Attachment #8989558 - Flags: review?(enndeakin) → review+
Neil,
Just want to make sure you're alright with the new changes^
Flags: needinfo?(enndeakin)
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2d89deed396
Support tooltips in top level non-XUL windows. r=enndeakin+6102,mats+6102
I'm going to out of the office the next few weeks, so I went ahead and landed the patch to unblock Brian. Brian mentioned he's fine to do any follow up if there's anything while I'm away.
Flags: needinfo?(enndeakin)
https://hg.mozilla.org/mozilla-central/rev/b2d89deed396
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.