280541 - XUL element tooltiptext doesn't work in non-chrome

Status: NEW

(Core :: XUL, defect)

Description

[Stephan Schulz]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.3; Linux) KHTML/3.3.91 (like Gecko)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b) Gecko/20050119

The XUL element tooltiptext doesn't work. I can't see my tooltip only a empty  
rectangle.. With stable Firefox/Mozilla works fine. 

Reproducible: Always

Steps to Reproduce:
1. add tooltiptext to e.g. your xul-button 
2. .. 
3.

Comment 1

[Neil Deakin]

You need to provide a testcase or URL. Reopen the bug and post it here if you
have one.

Comment 2

[Stephan Schulz]

Attachment: Test Tooltiptext

Comment 3

[Stephan Schulz]

Add testcase..  

Comment 4

[Boris Zbarsky [:bzbarsky]]

Interesting... This was broken by the checkin for bug 165110.  I tested, and
making XULElement implement IsNativeAnonymous() again did NOT help.  Overriding
SetNativeAnonymous to do nothing did, though.

That somewhat confuses me, since those are the only two methods that look at the
generic element anonymous flag...

In any case, it looks like the tooltip is creating anonymous content that the
document then can't access for security reasons or something?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, looks like I got the signature for IsNativeAnonymous() wrong in my test. 
Just adding that back in, without SetNativeAnonymous being a no-op, "fixes" the
bug...

Comment 6

[[e-mail]]

The popupshowing handler for <tooltip> calls this.getAttribute("default") and
sets this.label which is probably prohibited by caps because of the anonymity.
The Mac's scrollbars suffer from a similar problem in the Modern theme.

Comment 7

[[e-mail]]

Attachment: Set the label in C++

OK, so I didn't understand the <tooltip> XBL implementation ;-)
The default attribute changes are an optimization.
The .h changes are to fix an apparent oversight.
Comments?

Comment 8

[Boris Zbarsky [:bzbarsky]]

So the problem here is that we have a XUL binding attached to a native anonymous
node.  This binding is compiled with the security principal of the page in
question... and hence can't call methods on the node it's bound to.

That seems a little silly.  Should XUL bindings attached to native anonymous
content be compiled with the system principal perhaps (as Neil suggested)?  If
so, how do I arrange that, exactly?

Past that, I don't really know this code enough to tell what Neil's patch does...

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Compiling some bindings using the system principal is opening a whole can of
worms and I don't think we want to go there, at least not right now.

I don't know enough about this code to say if Neils patch is good.

I did think of another possible solution though, and that is to create the
tooltip nodes in such a way that they are not native-anonymous. I don't think
that they will be styled anyway since they are not part of the webpage DOM but
rather of the chrome DOM.

Comment 10

[Boris Zbarsky [:bzbarsky]]

sicking, we have a longstanding issue with scrollbars too, due to the same
issue.  See comment 6.  We need a general solution here, not just one for
tooltips...

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Agreed, but chaning the policy for which principal we compile xbl for is a very
risky change.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I agree.  We need something short-term for 1.8, and a long-term general solution....

Perhaps for 1.8 we want to just revert the changes to XUL that make it properly
support anonymous content-ness....

Comment 13

[[e-mail]]

Attachment: IsNativeAnonymous hack

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173970 [details] [diff] [review]
IsNativeAnonymous hack

That seems reasonable as a short-term hack for 1.8b.   We still need a
long-term solution, since this is basically re-opening up the "web pages can
access native anonymous content" hole in XUL... (which we've had for forever,
of course).

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Won't this make stylerules in webpage stylesheets apply to scrollbars?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173970 [details] [diff] [review]
IsNativeAnonymous hack

Erm... yes, it would.  :(  Of course, if scrollbars are native anonymous they
go back to being broken on Mac...

Comment 17

[[e-mail]]

And just how did that work before we actually supported IsNativeAnonymous?

Comment 18

[Boris Zbarsky [:bzbarsky]]

We used to explicitly claim that scrollbars were native anonymous
(IsNativeAnonymous() checked the tagname).

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

What would happen if we made IsNativeAnonymous return

nsGenericElement::IsNativeAnonymous && mName->Equals(nsXULAtoms::scrollbar)

Would mac scrollbars still be broken? I suspect they would...

Comment 20

[Boris Zbarsky [:bzbarsky]]

Yes, they would.

Comment 21

[[e-mail]]

(In reply to comment #18)
>We used to explicitly claim that scrollbars were native anonymous
Only since bug 251197. Mac scrollbars have been broken ever since.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So do scrollbars have to be nativeanonymous? Isn't it enough that they are
anonymous? (anonymous content doesn't by default get styled by author
stylesheet, right?). So could that be the right longterm solution for both
tooltips and scrollbars.

Originally nativeanonymous was created to block pages from reaching into an
<input type=file> and modifying its anonymous contents directly. However maybe
things were taken a bit too far? It feels silly to limit access to things like
scrollbars and tooltips that the page could just create itself and do whatever
it wanted with.

On the other hand, i don't think that the code for our various anonymous content
has been security checked to make sure that they can handle people poking around
in them. Though I can't off the top of my head think of any dangers other the
the fileupload control.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173970 [details] [diff] [review]
IsNativeAnonymous hack

Actually, I recall now.

Author stylesheet styling is controlled by the bindingParent of the node.  If
node->GetBindingParent() == node, it's not styled by author sheets (which is a
hack in and of itself, btw).

So the IsNativeAnonymous Neil attached would in fact not cause XUL scrollbars
to be styled, since we still set up the bindingParent on them in this hacky
way...

So reinstating my reviews and re-requesting approval.

Jonas, I agree that we should work out exactly what it means to be "native
anonymous", how such content should be treated (eg, when should the presshell
be told about it?), etc.  All of this is badly underspecified, right now, and
we have at least three radically distinct types of "native anonymous" content
at the moment (that's if you count the innards of an <input> and the scrollbars
as one type; the other two are CSS generated content and XTF content).

Comment 24

[Asa Dotzler [:asa]]

Comment on attachment 173970 [details] [diff] [review]
IsNativeAnonymous hack

a=asa for 1.8b checkin

Comment 25

[[e-mail]]

Wallpapered over for 1.8b

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could this have been responsible for the 4ms (0.5%) Tp increase on btek?

Comment 27

[Simon Fraser [no longer active]]

Why are the scrollbars in the Mac modern skin any different from the scrollbars
in the modern skin on other platforms?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Because of
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/bindings/scrollbar.xml#38

Comment 29

[Simon Fraser [no longer active]]

And, why, pray, does mac need to be different here:

if (navigator.platform.indexOf("Mac") != -1)
  this.initScrollbar();

Blake added this for bug 113582, but I'm none the wiser. Look, for example, at
bug 113582 comment #2.

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If mac indeed does need to keep its special scrollbar code an alternative
approach would be to implement it in cpp rather then in xbl.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> And, why, pray, does mac need to be different here:

Because Mac is the only platform that supports those different scrollbar styles?
 All the other platforms always report "single" for the style.

Comment 32

[[e-mail]]

Neil, echoing comment 26, is there any concern over the 0.5% pageload increase
this seems to have caused on btek?

Comment 33

[[e-mail]]

What, by implementing a virtual function override that does less than the
function it overrides? I fail to see how that can increase Tanything...

Comment 34

[[e-mail]]

btek might be smoking something... perhaps it's worth backing out for a while to
see if this patch is the culprit?

Comment 35

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Well, we might end up doing more work in other places due to xulelements no
longer being anonymous.

However since this patch is basically just a backout of a previous patch, if it
did cause a txul regression it is regressing a gain that we recently got.

Though this wasn't a straight backout. Maybe that could have made a difference?

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Could we do this by modifying userChrome.css rather then the current script hackery?

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, an even simpler solution is to have several different bindings and in C++ code decide which of them to use.

Comment 38

[[e-mail]]

Or we could move that scrollbar code into C++, #ifdef XP_MACOSX

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You mean go into the anonymous tree and call SetAttr every time we instantiate a scrollbar?

Comment 40

[[e-mail]]

(In reply to comment #39)
>You mean go into the anonymous tree and call SetAttr every time we instantiate
>a scrollbar?
If we called SetAttr every time it wouldn't be moving the JS code into C++ it would be writing a completely different implementation.

Comment 41

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Isn't that what the current JS does?
http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/scrollbar.xml#37

How else would we write this stuff in C++?

Comment 42

[[e-mail]]

(In reply to comment #41)
>Isn't that what the current JS does?
Not every time; only for a nondefault look.

Comment 43

[Boris Zbarsky [:bzbarsky]]

I just posted to m.d.t.dom about this.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Attachment 173247 [details] WFM on the trunk. Is this bug OK to be resolved or is more work still needed?

Comment 45

[davebrig]

I tested the attachments above on Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3

The tooltip appears on the button, but it reports an error in line 183 chrome://global/content/bindings/popup.xml. There is a permission-problem with XULElement.firstChild.