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

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Brian Grinstead [:bgrins]]

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.

Comment 1

[Brendan Dahl [:bdahl]]

Attachment: Bug 1460691 - Support tooltips in top level non-XUL windows. +6102

Add an anonymous XUL tooltip node to toplevel non-XUL windows. Setup a
nsXULTooltipListener on non-XUL nsXULWindows. Make nsXULTooltipListener
always use the default tooltip in the non-XUL case.

Review commit: https://reviewboard.mozilla.org/r/254586/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254586/

Comment 2

[Mats Palmgren (inactive)]

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.

Comment 3

[Brendan Dahl [:bdahl]]

Comment on attachment 8989558 [details]
Bug 1460691 - Support tooltips in top level non-XUL windows. +6102

See above^

Comment 4

[Brendan Dahl [:bdahl]]

Comment on attachment 8989558 [details]
Bug 1460691 - Support tooltips in top level non-XUL windows. +6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254586/diff/1-2/

Comment 5

[Neil Deakin]

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?

Comment 6

[Brendan Dahl [:bdahl]]

Comment on attachment 8989558 [details]
Bug 1460691 - Support tooltips in top level non-XUL windows. +6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254586/diff/2-3/

Comment 7

[Brendan Dahl [:bdahl]]

Neil,
Just want to make sure you're alright with the new changes^

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2d89deed396
Support tooltips in top level non-XUL windows. r=enndeakin+6102,mats+6102

Comment 9

[Brendan Dahl [:bdahl]]

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.

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b2d89deed396