1359303 - stylo: should not skip parent display-based style fixups for NAC that is not a NAC root

Status: RESOLVED FIXED

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

Description

[Cameron McCormack (:heycam)]

From bug 1344914 comment 5, bz points out that Servo is currently skipping parent display-based style fixups for all NAC, whereas we probably only want to do this for NAC roots (and after bug 1358968, anonymous boxes marked as skipping this fixup).

Comment 1

[Jeremy Chen ([e-mail])]

I'll investigate this.

Comment 2

[Jeremy Chen ([e-mail])]

Before I dig deeper into the implementation, I'd like to know if there's a good way to verify if this has already been resolved or not.

Hi Boris, any idea about making an automatic/manual test to do the verification? I can't find one in our repo, I guess it is a bit tricky to test the NAC. The worst case might be writing some C++ codes to generate multi-level NACs and print some logs to verify....

Comment 3

[Emilio Cobos Álvarez (:emilio)]

This is not fixed, per:

    #[inline]
    fn skip_root_and_item_based_display_fixup(&self) -> bool {
        // We don't want to fix up display values of native anonymous content.
        // Additionally, we want to skip root-based display fixup for document
        // level native anonymous content subtree roots, since they're not
        // really roots from the style fixup perspective.  Checking that we
        // are NAC handles both cases.
        self.is_native_anonymous()
    }

Changing that line should be enough, I think.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

That being said, NAC are under our control, so I'm not sure if we really need to fix this proper. Seems like an easy fix of course, but... :-)

Comment 5

[Bobby Holley (:bholley)]

Yeah, looking at this again I think it's probably not worth spending too much time on right now.

IIUC we should change the code in comment 3 to check:

self.is_native_anonymous() && (self.is_root_of_native_anonymous_subtree() || self.get_implement_pseudo_element().is_some())

So let's just write that patch (get review from bz or heycam), and not worry about testing it.

Comment 6

[Jeremy Chen ([e-mail])]

Attachment: Bug 1359303 - stylo: do not skip parent display-based style fixups for NAC that is not a NAC root.

Stylo is currently skipping parent display-based style fixups for all NAC,
whereas we probably only want to do this for NAC roots.

In this patch, we ensure that we skip it for NAC roots, and pseudo-elements,
but not other NAC.

Review commit: https://reviewboard.mozilla.org/r/174082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174082/

Comment 7

[Jeremy Chen ([e-mail])]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> This is not fixed, per:
> 
>     #[inline]
>     fn skip_root_and_item_based_display_fixup(&self) -> bool {
>         // We don't want to fix up display values of native anonymous
> content.
>         // Additionally, we want to skip root-based display fixup for
> document
>         // level native anonymous content subtree roots, since they're not
>         // really roots from the style fixup perspective.  Checking that we
>         // are NAC handles both cases.
>         self.is_native_anonymous()
>     }
> 
> Changing that line should be enough, I think.

Yes, that's the place I thought I should fix. Just don't know how to test the patch though...

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> Yeah, looking at this again I think it's probably not worth spending too
> much time on right now.
> 
> IIUC we should change the code in comment 3 to check:
> 
> self.is_native_anonymous() && (self.is_root_of_native_anonymous_subtree() ||
> self.get_implement_pseudo_element().is_some())
> 
> So let's just write that patch (get review from bz or heycam), and not worry
> about testing it.

Ok, then I'll just fix it and move on to other bugs.
Emilio and Bobby, thanks for the help. :)

Comment 8

[Boris Zbarsky [:bzbarsky]]

I wonder whether this bug would manifest with things in a <foreignObject> used via <use> that need display-based fixup...  That's where I'd start trying to write a testcase, at least.

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8902507 [details]
Bug 1359303 - stylo: do not skip parent display-based style fixups for NAC that is not a NAC root.

https://reviewboard.mozilla.org/r/174082/#review179392

Comment 10

[Jeremy Chen ([e-mail])]

Attachment: Servo PR, #18319

Comment 11

[Jeremy Chen ([e-mail])]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #8)
> I wonder whether this bug would manifest with things in a <foreignObject>
> used via <use> that need display-based fixup...  That's where I'd start
> trying to write a testcase, at least.

Thanks for the pointer. I'll try this today.
But if I can't easily make one, maybe we could leave the test as a followup bug.

Comment 12

[Jeremy Chen ([e-mail])]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #10)
> Created attachment 8903005 [details] [review]
> Servo PR, #18319

https://hg.mozilla.org/integration/autoland/rev/556f8f9a72c4

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #11)
> Thanks for the pointer. I'll try this today.
> But if I can't easily make one, maybe we could leave the test as a followup
> bug.

I haven't managed to make one at the moment, so I'll resolve this bug once the patch is merged to m-c.

Comment 13

[Jeremy Chen ([e-mail])]

https://hg.mozilla.org/mozilla-central/rev/556f8f9a72c4

Comment 14

[Boris Zbarsky [:bzbarsky]]

OK.  I just tried a testcase using <use> and it seems to work, so maybe use-cloned nodes are not marked native anonymous or something...