135895 - XHTML <style> element not working

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[John Keiser (jkeiser)]

In the build as of the 3rd, attachment 75083 [details] was showing up with lots of style.
 Today the styles specified in the page seem to not show up at all.  The only
thing "out of the ordinary" on this page is that it is XHTML Strict.

Comment 1

[John Keiser (jkeiser)]

Oh, some people believe this is a regression of bug 34849, which seems likely.

Comment 2

[Peter Van der Beken [:peterv]]

Yes. The XML content sink is blocking the load of the stylesheet for STYLE
elements with inline style (it used to handle those on its own). I've removed
the blocking (InitStyleLinkElement(nsnull, PR_TRUE)) and moved the updating of
the stylesheet from StartElement to EndElement, when we know that the STYLE
element has all of its children (including the inline style's textnode). I've
also made the HTMLStyleElement always mark itself as inline style if it's in an
XML document, we used to prevent <style src="..."> for XHTML in the XML content
sink but now that the element does all the style loading on its own, it has to
be handled there. Looking for r and sr on the patch I'm about to attach.

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v1

Eurk, wrong patch.

Comment 5

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

Comment on attachment 78021 [details] [diff] [review]
v1

>+    nsCOMPtr<nsIXMLDocument> xmlDoc = do_QueryInterface(mDocument);
>+    if (xmlDoc) {
>+      // We stopped supporting <style src="..."> for XHTML as it is
>+      // non-standard.
>+      *aIsInline = PR_TRUE;
>+      return;
>+    }

Don't check for xhtml-ness this way, do it by looking at the namespace in the
mNodeInfo which is kNameSpaceID_None for html and kNameSpaceID_HTML for xhtml.

other then that, r=sicking

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 78083 [details] [diff] [review]
v1.1

Moving forward r=sicking.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 78083 [details] [diff] [review]
v1.1

- In nsHTMLStyleElement.cpp:

+  if (mNodeInfo->NamespaceEquals(kNameSpaceID_HTML)) {

Make that kNameSpaceID_XHTML.

sr=jst

Comment 9

[Asa Dotzler [:asa]]

Comment on attachment 78083 [details] [diff] [review]
v1.1

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 10

[Jaime Rodriguez, Jr.]

What can this break, and what testing have you done to ensure this lands without
issue?

Comment 11

[Jaime Rodriguez, Jr.]

Pls land this on the trunk to bake for a few days, have QA look, and make sure
there are no regressions introduced, then come back to us for approval - ADT.
Removing adt1.0.0. Pls renominate once it has been on the trunk a couple days,
and been tested. 

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 136852 has been marked as a duplicate of this bug. ***

Comment 13

[Peter Van der Beken [:peterv]]

Checked in to the trunk. Leaving open so we can renominate for branch and ADT.

Comment 14

[Hixie (not reading bugmail)]

Looks good.

Having this fix means we pass over 100 more tests on the W3C Selectors test
suite. Without this fix, XHTML is basically pointless.

VERIFIED FIXED on the trunk.

Comment 15

[Peter Van der Beken [:peterv]]

I've gone through all of the open bugs filed since this landed, none of them
seem to be caused by these changes. I've mailed [e-mail] asking if he
could do some additional testing like he did for bug 34849, no reponse yet.

Comment 16

[scottputterman]

amar, could you test this?  It looks like this passes the Selector test.  What
else can be affected in html or xml and what test cases need to be run?

Comment 17

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: external style

Comment 18

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: testcase: HTML style src

The patch does not affect HTML. This testacase demonstrates it by showing that
the part of the patch that is outside of XML-only code is not affecting HTML
(style src should still work in HTML)

Comment 19

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: corrected href in testcase

Sorry, wrong stylesheet link, this is the correct one.

Comment 20

[scottputterman]

adding adt1.0.0+.  Please checkin to the branch as soon as possible and add the
fixed1.0.0 keyword.

Comment 21

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

*** Bug 139238 has been marked as a duplicate of this bug. ***

Comment 22

[Moied]

Testcases listed in this bug and related bugs passed,
Verified fix checked in cvs(rev 1.43)

Comment 23

[Peter Van der Beken [:peterv]]

Checked in to branch.

Comment 24

[Amarendra Hanumanula]

 I have tested HTML-Tables and Frames testcases which uses CSS for regressions
on trunk build 2002-04-14-11-trunk. All the testcases looks fine, no regressions.

Comment 25

[Heikki Toivonen (remove -bugzilla when emailing directly)]

And Hixie verified on trunk earlier, marking as such.

Comment 26

[Madhur Bhatia]

Checked this on win2000 build ID: 2002-05-22-1.0.0 branch 

I ran attachment 75083 [details], it is fixed on the branch.
The CSS selector testcases are also working fine.

Changing kweyword fixed1.0.0 to verified1.0.0