Closed Bug 1379901 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: cpeterson, Assigned: xidorn)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(2 files, 5 obsolete files)

reduced test
164 bytes, text/html
Details
Bug 1379901 - Update reftest expectation.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
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.
URL: https://crash-stats.mozilla.com/search/
Has STR: --- → yes
Version: unspecified → Trunk
Interestingly it appears like that in Chrome, too.
Attached file 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).
Since you've been looking at blockification and anonymous boxes, would you like to look at this bug Xidorn?
Flags: needinfo?(xidorn+moz)
We probably should figure out what's the correct behaviour per spec though...
(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.
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...
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
Attachment #8885982 - Attachment is obsolete: true
Attachment #8885982 - Flags: review?(emilio+bugs)
Attachment #8885983 - Attachment is obsolete: true
Attachment #8885983 - Flags: review?(emilio+bugs)
Attachment #8885984 - Attachment is obsolete: true
Attachment #8885984 - Flags: review?(emilio+bugs)
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)`?
Attachment #8886020 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8886021 [details]
Bug 1379901 - Update reftest expectation.

https://reviewboard.mozilla.org/r/156796/#review161940
Attachment #8886021 - Flags: review?(emilio+bugs) → review+
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?
Attachment #8886019 - Flags: review?(emilio+bugs)
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.
Attachment #8886019 - Attachment is obsolete: true
Attachment #8886020 - Flags: review+ → review?(emilio+bugs)
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?
Attachment #8886020 - Flags: review?(emilio+bugs) → review+
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.
Hmmm... the new version of the patches doesn't work anymore...
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.
Attachment #8886020 - Flags: review?(cam)
This change would conflict with bug 1378287 so I'd probably need to wait for that to land first.
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?
Attachment #8886020 - Flags: review?(cam) → review+
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.
Attachment #8886020 - Attachment is obsolete: true
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3df984fd0ba2
Update reftest expectation. r=emilio
https://hg.mozilla.org/mozilla-central/rev/3df984fd0ba2
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: