Last Comment Bug 745712 - FillInHTMLTooltip should not use title attributes from XUL ascendants
: FillInHTMLTooltip should not use title attributes from XUL ascendants
Status: RESOLVED FIXED
[qa-]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 14
Assigned To: Tim Taubert [:ttaubert]
:
:
Mentors:
Depends on:
Blocks: 601091 736476
  Show dependency treegraph
 
Reported: 2012-04-16 04:37 PDT by Tim Taubert [:ttaubert]
Modified: 2012-06-04 14:34 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch v1 (2.00 KB, patch)
2012-04-16 04:37 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v2 (2.01 KB, patch)
2012-04-16 05:26 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2012-04-16 04:37:55 PDT
Created attachment 615302 [details] [diff] [review]
patch v1

When hovering the <div> in this example:

<xul:window title="foobar">
  <div></div>
</xul:window>

FillInHTMLTooltip will not find a "title" attribute on the div itself and thus walk up its parents until one of them has a title set. We need to ignore XUL parents when doing this.
Comment 1 Dão Gottwald [:dao] 2012-04-16 04:52:09 PDT
Comment on attachment 615302 [details] [diff] [review]
patch v1

>   // Don't show the tooltip if the tooltip node is a XUL element, a document or is disconnected.
>-  if (tipElement.namespaceURI == "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" ||
>+  if (tipElement.namespaceURI == XULNS ||

Does this check still make sense?
Comment 2 Tim Taubert [:ttaubert] 2012-04-16 04:58:31 PDT
(In reply to Dão Gottwald [:dao] from comment #1)
> Does this check still make sense?

If we remove this check then we'd see a tooltip in case we hover the <xul:vbox> in this example:

<div title="foobar">
  <xul:vbox></xul:vbox>
</div>

So I think it make sense to remove it. What do you think?
Comment 3 Tim Taubert [:ttaubert] 2012-04-16 05:26:34 PDT
Created attachment 615308 [details] [diff] [review]
patch v2

Removed the first check.
Comment 4 Dão Gottwald [:dao] 2012-04-16 07:59:26 PDT
Comment on attachment 615308 [details] [diff] [review]
patch v2

>   // Don't show the tooltip if the tooltip node is a XUL element, a document or is disconnected.
>-  if (tipElement.namespaceURI == "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" ||
>-      !tipElement.ownerDocument ||
>+  if (!tipElement.ownerDocument ||

// Don't show the tooltip if the tooltip node is a document or disconnected.

>+  let XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";

const
Comment 5 Tim Taubert [:ttaubert] 2012-04-16 09:20:26 PDT
https://hg.mozilla.org/integration/fx-team/rev/1e7f193ff1a7
Comment 6 Rob Campbell [:rc] (:robcee) 2012-04-17 10:48:13 PDT
https://hg.mozilla.org/mozilla-central/rev/1e7f193ff1a7
Comment 7 Tim Taubert [:ttaubert] 2012-04-19 02:13:52 PDT
Comment on attachment 615308 [details] [diff] [review]
patch v2

[Approval Request Comment]
Regression caused by (bug #): no regression
Testing completed (on m-c, etc.): landed on m-c some days ago
Risk to taking this patch (and alternatives if risky): very small patch and risk
String changes made by this patch: none

We need this as a prerequisite for bug 736476.
Comment 8 Tim Taubert [:ttaubert] 2012-04-20 23:27:39 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/621430357bac
Comment 9 Ioana (away) 2012-05-31 04:58:32 PDT
Verified on:
Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20100101 Firefox/13.0
BuildID: 20120528154913

1.
<xul:window title="foobar">
  <div>divvvvv</div>
</xul:window>

Results: "foobar" is displayed when hovering over the div.

2.
<xul:window title="foobar">
  <div title="title11">divvvvv</div>
</xul:window>

Results: "title11" is displayed when hovering over the div.

3.
<div title="foobar">
  <xul:vbox>vboxxxxx</xul:vbox>
</div>

Results: "foobar" is displayed when hovering over the vbox.

Tim, are these results as expected? At point 1, shouldn't there be not title displayed?
Comment 10 Tim Taubert [:ttaubert] 2012-05-31 07:11:11 PDT
(In reply to Ioana Budnar [QA] from comment #9)
> Tim, are these results as expected? At point 1, shouldn't there be not title
> displayed?

(1) is the same structure as we have it on the newtab page and it shouldn't show the title. How exactly did you test these code snippets?
Comment 11 Ioana (away) 2012-05-31 07:14:33 PDT
(In reply to Tim Taubert [:ttaubert] from comment #10)
> (In reply to Ioana Budnar [QA] from comment #9)
> > Tim, are these results as expected? At point 1, shouldn't there be not title
> > displayed?
> 
> (1) is the same structure as we have it on the newtab page and it shouldn't
> show the title. How exactly did you test these code snippets?

I added them in a html file that I loaded in Firefox. Is there another way you need me to test them?
Comment 12 Tim Taubert [:ttaubert] 2012-05-31 07:18:15 PDT
(In reply to Ioana Budnar [QA] from comment #11)
> I added them in a html file that I loaded in Firefox. Is there another way
> you need me to test them?

Since remote XUL got disabled I think this is not seen as a "real" XUL element but rather a HTML element with a custom namespace. You'd probably need to modify a XUL file shipped with Firefox or included via some add-on.
Comment 13 juan becerra [:juanb] 2012-06-02 15:39:15 PDT
I tried using the "remote xul manager" add-on and some examples for xul windows, but I wasn't able to show the tooltip for the divs in the first place, but if you have other suggestions, I could take another look at verifying this.
Comment 14 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-04 14:34:38 PDT
QA attempts to reproduce this so we can accurately verify the fix have been unsuccessful. Untracking for QA verification. If there is someone who can reproduce this bug, please assist us in the verification; we need this tested in Firefox 13. If you can provide QA a test to reproduce, please set the whiteboard tag back to [qa+].

Thank you.

Note You need to log in before you can comment on or make changes to this bug.