1406875 - stylo: Figure out how quirks mode and XBL stylesets should interact

Status: RESOLVED WONTFIX

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

Bug 1405543 happens because a quirks-mode mismatch between the content the binding is bound to, and the quirks mode of the binding styleset.

I think that for this to happen the binding needs to be "moved" or reused, because with a dumb test-case I didn't manage to repro the bug.

Anyway, I think we should be consistent about this. Maybe this is just a matter of handling the quirks-mode change properly in Servo_StyleSet_SetDevice. Or maybe XBL stylesets should always be NoQuirks.

I didn't do any of that in https://github.com/servo/servo/pull/18794 because it was safer to keep the previous behavior, but however we fix this, we can revert that afterwards (replacing it with an assert, maybe).

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Hy TYLin, what do you think about comment 0? I think just handling it in SetDevice should be ok, but wanted your opinion anyway.

Also, if you know how to trigger a binding "move" easily, we can test bug 1405543.

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #0)
> Bug 1405543 happens because a quirks-mode mismatch between the content the
> binding is bound to, and the quirks mode of the binding styleset.

Yes, this is the cause. MediaDocument inherits from nsHTMLDocument, and nsHTMLDocument uses quirks mode [1].

> I think that for this to happen the binding needs to be "moved" or reused,
> because with a dumb test-case I didn't manage to repro the bug.
> 
> Anyway, I think we should be consistent about this. Maybe this is just a
> matter of handling the quirks-mode change properly in
> Servo_StyleSet_SetDevice. Or maybe XBL stylesets should always be NoQuirks.

XBL bindings use the device (PresContext) of the first document that loads it, so the quirk mode associated with XBL stylesets is the one of the first document. I guess we need something similar to media features change to update the quirk mode of the XBL stylesets.

If we force XBL styleset to be always NoQuirks, I guess we still have an issue with video in Quirks document, right?

> I didn't do any of that in https://github.com/servo/servo/pull/18794 because
> it was safer to keep the previous behavior, but however we fix this, we can
> revert that afterwards (replacing it with an assert, maybe).

Yes. Once we have a proper fix, we should add an assertion to guarantee that the quirks mode of the XBL bindings matches the documents.

[1] http://searchfox.org/mozilla-central/rev/b53e29293c9e9a2905f4849f4e3c415e2013f0cb/dom/html/nsHTMLDocument.cpp#191

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #2)
> If we force XBL styleset to be always NoQuirks, I guess we still have an
> issue with video in Quirks document, right?

Well, I mean, we'd still need that special-case, but my point is that I'm not sure if people touching the video controls have quirks-mode into account in general, so a more convenient (and fast) behavior would be to just use NoQuirks for those.

But I agree that handling the move properly is nicer, and allows to get rid of that patch completely.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Taking since this is pretty annoying (see the blocking bug), and I think it'd be easier to just fix it properly.

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> Well, I mean, we'd still need that special-case, but my point is that I'm
> not sure if people touching the video controls have quirks-mode into account
> in general, so a more convenient (and fast) behavior would be to just use
> NoQuirks for those.

Totally agree. I don't see the reason to have MediaDocument in quirk mode. If we make MediaDocument in standard mode, the video bindings loaded by video or audio files will be in standard mode, which is more satisfying :)

Comment 6

[Emilio Cobos Álvarez (:emilio)]

So, the more I look at this, the less I understand how this is supposed to work.

So we store the resources in the prototype binding.

But that is a shared object among all the elements that get that binding for a given URI, right?

However, that binding is definitely stateful and dependent on the document state (we see this with quirks mode, but it also affects media queries, etc).

So if my understanding is right, each time a document changes we update the prototype resources with its state, which means, effectively, that all the other bindings reuse that state for the next restyle, unless it's updated first.

TYLin, is my understanding of this setup right? If so, how have we got away with it so easily? This looks pretty borked to me.

We should either store the state for each bound element (kinda wasteful), cache it somehow, as we do for the UA data cache, but with an extra key for the quirks mode, I guess, or something like that...

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Totally agree. I don't see the reason to have MediaDocument in quirk mode. 

Well, apart from compat with ourselves, other browsers, and whatever the HTML spec says?  This is very much web-observable, so needs to be specified there.

On the bigger question of XBL and quirks mode, Gecko's behavior (pre-stylo) is that the quirks mode is stored on a per-RuleCascadeData level (well, only in members of RuleCascadeData).  And it looks like the loop in nsCSSRuleProcessor::RefreshRuleCascade just calls nsMediaQueryResultCacheKey::Matches which does not check the quirks mode.  So it looks like in Gecko we can end up with the mismatch described here for quirks mode (but not media features, etc).  How does that work in Gecko?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fwiw, I filed https://github.com/whatwg/html/issues/3113 on the spec end of quirkiness here.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

I'll fix the invalidation stuff to use the same quirks mode as the XBL stuff, in the same spirit as https://github.com/servo/servo/pull/18794

After that, I'll figure out how to fix this properly.

Comment 10

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> So, the more I look at this, the less I understand how this is supposed to
> work.
> 
> So we store the resources in the prototype binding.
> 
> But that is a shared object among all the elements that get that binding for
> a given URI, right?

Yes. All the elements with the same binding URI share the binding.

> However, that binding is definitely stateful and dependent on the document
> state (we see this with quirks mode, but it also affects media queries, etc).

Right.

> So if my understanding is right, each time a document changes we update the
> prototype resources with its state, which means, effectively, that all the
> other bindings reuse that state for the next restyle, unless it's updated
> first.

If I recall correctly, each XBL document will loaded once and be cached here [1]. That is, whenever a new document loads an XBL bindings, it will use the cached XBL document as well as its prototype resources. Currently, with stylo, the only update to the bindings is through media query changes.

> TYLin, is my understanding of this setup right? If so, how have we got away
> with it so easily? This looks pretty borked to me.

> We should either store the state for each bound element (kinda wasteful),
> cache it somehow, as we do for the UA data cache, but with an extra key for
> the quirks mode, I guess, or something like that...

If we don't endup implementing a proper cache. Perhaps we can cache something like ServoStyleSet::mLastQuirksMode, and have ServoStyleSet::CompatibilityModeChanged() calls nsBindingManager to update all the bindings when the quirks mode changes. However, the video test case has quirk mode by default, which won't trigger CompatibilityModeChanged(). We need to think about how to cope with that. 

[1] http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/dom/xbl/nsXBLService.cpp#932

Comment 11

[Emilio Cobos Álvarez (:emilio)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #10)
> If we don't endup implementing a proper cache. Perhaps we can cache
> something like ServoStyleSet::mLastQuirksMode, and have
> ServoStyleSet::CompatibilityModeChanged() calls nsBindingManager to update
> all the bindings when the quirks mode changes. However, the video test case
> has quirk mode by default, which won't trigger CompatibilityModeChanged().
> We need to think about how to cope with that.

That would be pretty bogus in a lot of senses. Dynamic changes to the bindings will start or stop working depending on the last document that changes quirks mode.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

The fix for the invalidation stuff with mismatched quirks modes is https://github.com/servo/servo/pull/18826.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

I don't think we plan to fix this. What we have now works for our use cases, XBL is going away, and Shadow DOM no longer uses XBL and thus is sane.