1066554 - [shadow-dom] The mathml.css stylesheet is not loaded when inserting shadow MathML

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Frédéric Wang (:fredw)]

Attachment: testcase.html

In the attached testcase, if you click "insert Shadow Math" you see a "X^X" formula with the exponent that incorrectly has the same size as the base. If you click "insert Math", you then get the correct rendering for the shadow formula. This would suggest that mathml.css (which contains -moz-increment-script-level rules) is not loaded when inserting Shadow Math.

This used to work in Gecko 31 but is broken in nightly. I haven't tried to find a regression window, but bug 946065 might be a candidate.

Note: the use case was for the <x-tex> custom element http://fred-wang.github.io/TeXZilla/examples/customElement.html

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

My guess is that some code uses GetCurrentDoc() which is null for shadow dom.
It should use GetComposedDoc().

Where do we start loading mathml.css?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Oh, maybe it is a different issue
http://mxr.mozilla.org/mozilla-central/source/dom/mathml/nsMathMLElement.cpp#92

See bug 1062578.
Could you test the patch in that bug?

Comment 3

[James Kitchener (:jkitch)]

Last good revision: f5232023af37
First bad revision: a2f0e0619332
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f52320
23af37&tochange=a2f0e0619332

Comment 4

[Frédéric Wang (:fredw)]

(In reply to Olli Pettay [:smaug] from comment #2)
> See bug 1062578.
> Could you test the patch in that bug?

No, that does not seem to help :-(

(In reply to James Kitchener from comment #3)
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f5232023af37&tochange=a2f0e0619332

So maybe bug 992521...

Comment 5

[James Kitchener (:jkitch)]

Attachment: path that seems to work

The attached patch seems to fix it, however I do not know that bit of code well enough to be confident of correctness.

https://tbpl.mozilla.org/?tree=Try&rev=ce7b4a241b9d

Comment 6

[Boris Zbarsky [:bzbarsky]]

Please retest on top of the patch for bug 1062578?

Comment 7

[Boris Zbarsky [:bzbarsky]]

And in particular, we need a clear understanding of what exactly the nsIDocument argument to BindToTree is supposed to mean in the Shadow DOM world.  Just changing it ad-hoc in one particular spot definitely seems wrong.

Comment 8

[William Chen [:wchen]]

The nsIDocument passed to BindToTree is currently the uncomposed document, so it will be null for elements in a shadow tree.

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK.  Then we should probably change the mathml stylesheet loading bits somehow to look at the composed doc...

Comment 10

[Frédéric Wang (:fredw)]

OK, of course I should have said that before but we probably want something similar for other UA stylesheets:

http://dxr.mozilla.org/mozilla-central/search?q=EnsureOnDemandBuiltInUASheet

(mmmh, BTW shouldn't SVGDocument::EnsureNonSVGUserAgentStyleSheetsLoaded also load the MathML stylesheet? That is, can we use a MathML document to do something like bug 1016145?)

Comment 11

[Boris Zbarsky [:bzbarsky]]

> shouldn't SVGDocument::EnsureNonSVGUserAgentStyleSheetsLoaded also load the MathML
> stylesheet

No, since we have other codepaths that will load the mathml bits on demand.

Comment 12

[James Kitchener (:jkitch)]

Attachment: patch

The initial patch was very wrong.  The attached patch follows the suggestion from comment 9.

The effects should be limited to MathML elements.  Other issues involving SVG/XUL-specific stylesheets I would prefer to leave to someone with more experience in that area.

Comment 13

[James Kitchener (:jkitch)]

Attachment: patch

The patch wasn't refreshed before submission.

nsMathMLElement::UnbindFromTree() now uses the composed document as well to be consistent.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8488985 [details] [diff] [review]
patch

Please don't change the link-update behavior; that stuff should be done in concert with other link-related code that assumes certain behavior from bind/unbind.

And it really would be good to fix the other EnsureOnDemandBuiltInUASheet consumers too.  They should need pretty identical changes.  If you're not willing to do that, at the very least make sure there are followup bugs filed on them.

Comment 15

[James Kitchener (:jkitch)]

Attachment: patch

Review comments have been addressed.  

If the XUL and SVG changes require their own tests, I'll split them off into followup bugs.

Comment 16

[Frédéric Wang (:fredw)]

Attachment: svg-shadow.html

For the tests, you might also consider == reftests if that's easier (normal DOM vs shadow DOM) although I don't don't know if we can use XUL in reftest pages.

Here is a quick testcase for SVG.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8490024 [details] [diff] [review]
patch

Looks great, thanks!

A test for SVG would be good, yes.  Followup bug is OK.

I wouldn't worry too much about a test for XUL if it's being too annoying.

Comment 18

[James Kitchener (:jkitch)]

Attachment: patch

Thank you for the SVG test.

Both tests have been converted to reftests.

Comment 19

[James Kitchener (:jkitch)]

https://tbpl.mozilla.org/?tree=Try&rev=ec96a86ddf51

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf81dcd4302c

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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