The default bug view has changed. See this FAQ.

FillInHTMLTooltip should not use title attributes from XUL ascendants

RESOLVED FIXED in Firefox 13

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

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

Firefox Tracking Flags

(firefox13 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
Attachment #615302 - Flags: review?(dao)
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?
(Assignee)

Comment 2

5 years ago
(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?
(Assignee)

Comment 3

5 years ago
Created attachment 615308 [details] [diff] [review]
patch v2

Removed the first check.
Attachment #615302 - Attachment is obsolete: true
Attachment #615302 - Flags: review?(dao)
Attachment #615308 - Flags: review?(dao)
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
Attachment #615308 - Flags: review?(dao) → review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/1e7f193ff1a7
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/1e7f193ff1a7
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 7

5 years ago
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.
Attachment #615308 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Attachment #615308 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/621430357bac
status-firefox13: --- → fixed
Whiteboard: [qa+]
Blocks: 601091

Comment 9

5 years ago
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?
(Assignee)

Comment 10

5 years ago
(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

5 years ago
(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?
(Assignee)

Comment 12

5 years ago
(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.
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.
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.
Whiteboard: [qa+] → [qa-]
You need to log in before you can comment on or make changes to this bug.