1379901 - stylo: Socorro Super Search page's form fields are aligned vertically instead of horizontally

Status: RESOLVED FIXED

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

Description

[Chris Peterson [:cpeterson]]

STR:
1. Load Socorro Search Search page https://crash-stats.mozilla.com/search/

RESULT:
The Product, Version, Platform, and Process type form fields are aligned vertically instead of horizontally.

Comment 1

[Cameron McCormack (:heycam)]

Interestingly it appears like that in Chrome, too.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: reduced test

I think this should be a reduced test for this issue.

I feel like it's probably related to blockification (due to the display:flex) and anonymous boxes (due to the fieldset).

Comment 3

[Cameron McCormack (:heycam)]

Since you've been looking at blockification and anonymous boxes, would you like to look at this bug Xidorn?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

We probably should figure out what's the correct behaviour per spec though...

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Cameron McCormack (:heycam) from comment #1)
> Interestingly it appears like that in Chrome, too.

The form fields are also aligned vertically in Edge and Safari, but curiously they are aligned horizontally in IE11. So Microsoft switched their behavior between IE to Edge.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

This is the stylo version of bug 1230207. Specifically, I think the problem is that Stylo doesn't have the corresponding display value fixup in nsRuleNode::ComputeDisplayData added in that bug.

I can probably try fixing this...

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Record pseudo-element in computed::Context.

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

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Adjust display value for ;-moz-fieldset-content when parent is flex\grid.

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

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1379901 - Update reftest expectation.

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

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Pass pseudo-element into StyleAdjuster.

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

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Adjust display value for ::-moz-fieldset-content when parent is flex/grid.

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

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1379901 - Update reftest expectation.

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

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8886020 [details]
Adjust display value for ::-moz-fieldset-content when parent is flex/grid.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156794/diff/1-2/

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8886021 [details]
Bug 1379901 - Update reftest expectation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156796/diff/1-2/

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886020 [details]
Adjust display value for ::-moz-fieldset-content when parent is flex/grid.

https://reviewboard.mozilla.org/r/156794/#review161938

::: servo/components/style/style_adjuster.rs:298
(Diff revision 2)
> +    /// If a <fieldset> has grid/flex display type, we need to inherit
> +    /// this type into its ::-moz-fieldset-content anonymous box.
> +    #[cfg(feature = "gecko")]
> +    fn adjust_for_fieldset_content(&mut self, layout_parent_style: &ComputedValues,
> +                                   pseudo: Option<&PseudoElement>) {
> +        if pseudo.is_none() || *pseudo.unwrap() != PseudoElement::FieldsetContent {

if !matches!(pseudo, Some(&PseudoElement::FieldsetContent) {
    return;
}

::: servo/components/style/style_adjuster.rs:301
(Diff revision 2)
> +    fn adjust_for_fieldset_content(&mut self, layout_parent_style: &ComputedValues,
> +                                   pseudo: Option<&PseudoElement>) {
> +        if pseudo.is_none() || *pseudo.unwrap() != PseudoElement::FieldsetContent {
> +            return;
> +        }
> +        let parent_display = layout_parent_style.get_box().clone_display();

`debug_assert_eq!(self.style.get_box().clone_display(), display::block)`?

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886021 [details]
Bug 1379901 - Update reftest expectation.

https://reviewboard.mozilla.org/r/156796/#review161940

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886019 [details]
Pass pseudo-element into StyleAdjuster.

https://reviewboard.mozilla.org/r/156792/#review161942

This one looks slightly fishy...

::: servo/components/style/style_resolver.rs:491
(Diff revision 1)
>          }
>  
>          let values =
>              Arc::new(cascade(
>                  self.context.shared.stylist.device(),
> +                pseudo,

Also, this is not quite right, since only eager pseudos arrive here... Right now we dont test for other pseudos that arrive here, but if we want to make the code som generic, we should, probably... Otherwise we could just add a bit to CascadeFlags when cascading a MozFieldsetContents, but that seems suboptimal... Maybe it's easier?

::: servo/components/style/stylist.rs:1341
(Diff revision 1)
>          // This currently ignores visited styles.  It appears to be used for
>          // font styles in <canvas> via Servo_StyleSet_ResolveForDeclarations.
>          // It is unclear if visited styles are meaningful for this case.
>          let metrics = get_metrics_provider_for_product();
>          Arc::new(properties::cascade(&self.device,
> +                                     None,

This, and the other animation one look quite fishy... Nothing guarantees that this doesn't belong to a pseudo-element... Perhaps the style should store it?

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8886019 [details]
Pass pseudo-element into StyleAdjuster.

https://reviewboard.mozilla.org/r/156792/#review161942

> Also, this is not quite right, since only eager pseudos arrive here... Right now we dont test for other pseudos that arrive here, but if we want to make the code som generic, we should, probably... Otherwise we could just add a bit to CascadeFlags when cascading a MozFieldsetContents, but that seems suboptimal... Maybe it's easier?

So, the question is, if there is no reliable way to pass in the pseudo-element at all, how can we guarantee that we properly set the flag whenever we need? Adding a new cascade flag doesn't seem to change anything.

> This, and the other animation one look quite fishy... Nothing guarantees that this doesn't belong to a pseudo-element... Perhaps the style should store it?

This one is fine, since it is only for resolving style for canvas (font / filter). I have no idea about the animation.

The animation one already looks fishy in the same way, no? It passes empty CascadeFlags to apply_declarations. What would guarantee that the fixup would work as expected in that case?

In reality, I don't think this is something we really need to worry about, because anonymous boxes are not stylable from content, so no animation should be applied to them.

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8886020 [details]
Adjust display value for ::-moz-fieldset-content when parent is flex/grid.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156794/diff/2-3/

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8886021 [details]
Bug 1379901 - Update reftest expectation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156796/diff/2-3/

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886020 [details]
Adjust display value for ::-moz-fieldset-content when parent is flex/grid.

https://reviewboard.mozilla.org/r/156794/#review162014

::: servo/components/style/properties/properties.mako.rs:2553
(Diff revision 3)
>          /// is used by Gecko to prevent display:contents on generated
>          /// content.
>          const PROHIBIT_DISPLAY_CONTENTS = 0x10,
> +
> +        /// Whether we're styling the ::-moz-fieldset-content anonymous box.
> +        #[cfg(feature = "gecko")]

No need to cfg the flag, really...

::: servo/components/style/style_adjuster.rs:12
(Diff revision 3)
>  
>  use app_units::Au;
>  use properties::{self, CascadeFlags, ComputedValues};
>  use properties::{IS_ROOT_ELEMENT, SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP, StyleBuilder};
> +#[cfg(feature = "gecko")]
> +use properties::IS_FIELDSET_CONTENT;

Let's move this use to `adjust_for_fieldset_content`, to avoid the extra cfg check.

::: servo/components/style/style_adjuster.rs:299
(Diff revision 3)
> +    /// If a <fieldset> has grid/flex display type, we need to inherit
> +    /// this type into its ::-moz-fieldset-content anonymous box.
> +    #[cfg(feature = "gecko")]
> +    fn adjust_for_fieldset_content(&mut self, layout_parent_style: &ComputedValues,
> +                                   flags: CascadeFlags) {
> +        if flags.contains(IS_FIELDSET_CONTENT) {

Let's do:

```
if !flags.contains(IS_FIELDSET_CONTENT) {
    return;
}

// Rest
```

To be consistent with the other functions.

Also, I think fieldsets don't respect `display: contents`, so maybe you need to use `parent_style`, and leave a TODO for when display: contents is updated to the spec.

::: servo/components/style/stylist.rs:733
(Diff revision 3)
>  
> +        #[allow(unused_mut, unused_variables)]
> +        fn cascade_flags(pseudo: &PseudoElement) -> CascadeFlags {
> +            let mut flags = CascadeFlags::empty();
> +            #[cfg(feature = "gecko")] {
> +                if *pseudo == PseudoElement::FieldsetContent {

nit: I think this reads quite weirdly, usually we put the brace in the next line to the cfg:

```
#[cfg(feature = "gecko")]
{
    // ...
}
```

Anyway, this is fine, but you can also use a method in `PseudoElement`, similar to `is_before_or_after`, or `is_first_letter`, which would allow you to write this without the extra function, `allow`, and cfg checks, wdyt?

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886020 [details]
Adjust display value for ::-moz-fieldset-content when parent is flex/grid.

https://reviewboard.mozilla.org/r/156794/#review162014

> Let's do:
> 
> ```
> if !flags.contains(IS_FIELDSET_CONTENT) {
>     return;
> }
> 
> // Rest
> ```
> 
> To be consistent with the other functions.
> 
> Also, I think fieldsets don't respect `display: contents`, so maybe you need to use `parent_style`, and leave a TODO for when display: contents is updated to the spec.

A test-case for this would look like:

```
<div style="display: flex">
    <fieldset style="display: contents">
        <legend>Whatever</legend>
    </fieldset>
</div>
```

where the fieldset contents would be block in Gecko, but flex with your patch, IIUC.

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Hmmm... the new version of the patches doesn't work anymore...

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

Oh, hmmm, because anonymous boxes are actually styled in a different path... It turns out passing pseudo-element down is a more reliable way to handle this.

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8886020 [details]
Adjust display value for ::-moz-fieldset-content when parent is flex/grid.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156794/diff/3-4/

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8886021 [details]
Bug 1379901 - Update reftest expectation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156796/diff/3-4/

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

This change would conflict with bug 1378287 so I'd probably need to wait for that to land first.

Comment 28

[Cameron McCormack (:heycam)]

Comment on attachment 8886020 [details]
Adjust display value for ::-moz-fieldset-content when parent is flex/grid.

https://reviewboard.mozilla.org/r/156794/#review162766

::: servo/components/style/properties/properties.mako.rs:2552
(Diff revision 4)
> +        /// Whether we're styling the ::-moz-fieldset-content anonymous box.
> +        const IS_FIELDSET_CONTENT = 0x20,

I wonder whether a flag with a more generic name, like "INHERITS_FLEX_OR_GRID", might be better.  I guess there are no other anonymous boxes that Gecko needs to treat like this, but from the perspective of the browser-agnostic code here, it may make sense to not reference this Gecko-specific name.  (Happy to leave as is too.)

::: servo/components/style/style_adjuster.rs:302
(Diff revision 4)
> +        use properties::IS_FIELDSET_CONTENT;
> +        if !flags.contains(IS_FIELDSET_CONTENT) {
> +            return;
> +        }
> +        debug_assert_eq!(self.style.get_box().clone_display(), display::block);
> +        // TODO We actually wants style from parent rather than layout

Nit: s/wants/want/

::: servo/components/style/style_adjuster.rs:304
(Diff revision 4)
> +            return;
> +        }
> +        debug_assert_eq!(self.style.get_box().clone_display(), display::block);
> +        // TODO We actually wants style from parent rather than layout
> +        // parent, so that this fixup doesn't happen incorrectly when
> +        // when <fieldset> has "display: contents".

If the fieldset is display:contents, will we even create a ::-moz-fieldset-content anonymous box?

Comment 29

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8886020 [details]
Adjust display value for ::-moz-fieldset-content when parent is flex/grid.

https://reviewboard.mozilla.org/r/156794/#review162766

> I wonder whether a flag with a more generic name, like "INHERITS_FLEX_OR_GRID", might be better.  I guess there are no other anonymous boxes that Gecko needs to treat like this, but from the perspective of the browser-agnostic code here, it may make sense to not reference this Gecko-specific name.  (Happy to leave as is too.)

Yeah, I think we can rename it when we have more case doing so. Until then, I think it is better to use a more restrictive name here.

> If the fieldset is display:contents, will we even create a ::-moz-fieldset-content anonymous box?

Yes, we do, because `<fieldset>` generates a specific frame regardless of display value, as far as it is not in a `display: none` subtree.

Comment 30

[Xidorn Quan [:xidorn] UTC+10]

Servo PR: servo/servo#17764

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8886021 [details]
Bug 1379901 - Update reftest expectation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156796/diff/4-5/

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3df984fd0ba2
Update reftest expectation. r=emilio

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3df984fd0ba2