Closed Bug 1408235 Opened 2 years ago Closed 2 years ago

stylo: style attribute on XUL element in XUL document is not handled

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(3 files)

(Note that this is filed for annotating tests in the progress of bug 1407847. The test is not going to fail without patch there, because XUL is still using Gecko backend at the moment.)

There are testcases failing because that style attribute is not handled on XUL elements.

I suppose that style attribute should always be handled by nsStyledElement, but maybe that class only handles namespace-less style attributes, but attributes in XUL documents are generally in XUL namespace, so probably handled elsewhere.
Priority: -- → P3
I suspect that the relevant code is https://searchfox.org/mozilla-central/rev/ed1d5223adcdc07e9a2589ee20f4e5ee91b4df10/dom/xul/nsXULElement.cpp#2331-2350 which apparently doesn't handle the Stylo case.

It is not clear to me whether it is enough to just have it parse with Servo, or there is other code needs to be added.
OK, I think just add the parsing code is enough.

It probably doesn't make much sense to share the parsing code in nsAttrValue for now, since there are much more logic involves in the latter. If we do want to share code between the two places, that should be done in a separate patch.
Assignee: nobody → xidorn+moz
Blocks: 1407863
Hmmm, not as simple as I thought...

We may not have backend type when in nsXULPrototypeElement::SetAttrAt, and there is even no guarantee that we would have that when the declaration reaches nsXULElement::MakeHeavyweight, so basically we don't know whether we should build a Gecko declaration block or a Servo one...
Cannot figure out an elegant way to fix this... Probably we can land bug 1407847 first and then rely on the fact that only document with system principal doesn't get stylo...

Anyway, I thought it would take less effort to fix this bug than annotating all related failures, but maybe I was wrong, so I decided to move forward on bug 1407847 for now.
Assignee: xidorn+moz → nobody
Trying the approach mentioned in comment 4.
Assignee: nobody → xidorn+moz
Duplicate of this bug: 1407863
Comment on attachment 8920538 [details]
Bug 1408235 part 1 - Move stylo checking logic into a separate function.

https://reviewboard.mozilla.org/r/191542/#review196732

::: layout/base/nsLayoutUtils.h:2563
(Diff revision 1)
>    static void Initialize();
>    static void Shutdown();
>  
>  #ifdef MOZ_STYLO
>    /**
> +   * Return stylo should be used for a given document URI and principal.

"Return whether"
Attachment #8920538 - Flags: review?(cam) → review+
Comment on attachment 8920539 [details]
Bug 1408235 part 2 - Parse XUL style attribute into stylo declaration block when the document should use stylo.

https://reviewboard.mozilla.org/r/191544/#review196734
Attachment #8920539 - Flags: review?(cam) → review+
Comment on attachment 8920540 [details]
Bug 1408235 part 3 - Update test expectation for this bug.

https://reviewboard.mozilla.org/r/191546/#review196736

Nice!
Attachment #8920540 - Flags: review?(cam) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s b9c95f1f2bf9 -d 067d0a4c0bed: rebasing 428730:b9c95f1f2bf9 "Bug 1408235 part 1 - Move stylo checking logic into a separate function. r=heycam"
merging layout/base/nsLayoutUtils.cpp
merging layout/base/nsLayoutUtils.h
rebasing 428731:8ebd8b9e35e5 "Bug 1408235 part 2 - Parse XUL style attribute into stylo declaration block when the document should use stylo. r=heycam"
rebasing 428732:77c5fb689dff "Bug 1408235 part 3 - Update test expectation for this bug. r=heycam" (tip)
merging layout/reftests/bidi/reftest.list
merging layout/reftests/bugs/reftest.list
merging layout/reftests/css-display/reftest.list
merging layout/reftests/xul/reftest.list
warning: conflicts while merging layout/reftests/bugs/reftest.list! (edit, then use 'hg resolve --mark')
warning: conflicts while merging layout/reftests/css-display/reftest.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/61e3281b53ae
part 1 - Move stylo checking logic into a separate function. r=heycam
https://hg.mozilla.org/integration/autoland/rev/46d06b3ab9e2
part 2 - Parse XUL style attribute into stylo declaration block when the document should use stylo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/4a0e5e6c2b73
part 3 - Update test expectation for this bug. r=heycam
You need to log in before you can comment on or make changes to this bug.