Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
10 days ago
3 days ago

People

(Reporter: cpeterson, Assigned: xidorn)

Tracking

(Blocks: 1 bug)

Trunk
mozilla56
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox56 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

10 days ago
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.

Updated

10 days ago
Has STR: --- → yes
Version: unspecified → Trunk
Interestingly it appears like that in Chrome, too.
Created attachment 8885628 [details]
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...
(Reporter)

Comment 5

9 days ago
(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.
(Assignee)

Comment 6

9 days ago
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 days ago
Attachment #8885982 - Attachment is obsolete: true
Attachment #8885982 - Flags: review?(emilio+bugs)
(Assignee)

Updated

9 days ago
Attachment #8885983 - Attachment is obsolete: true
Attachment #8885983 - Flags: review?(emilio+bugs)
(Assignee)

Updated

9 days ago
Attachment #8885984 - Attachment is obsolete: true
Attachment #8885984 - Flags: review?(emilio+bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

8 days ago
mozreview-review
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 16

8 days ago
mozreview-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 17

8 days ago
mozreview-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)
(Assignee)

Comment 18

8 days ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 days ago
Attachment #8886019 - Attachment is obsolete: true
(Assignee)

Updated

8 days ago
Attachment #8886020 - Flags: review+ → review?(emilio+bugs)

Comment 21

8 days ago
mozreview-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

::: 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 22

8 days ago
mozreview-review-reply
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.
(Assignee)

Comment 23

8 days ago
Hmmm... the new version of the patches doesn't work anymore...
(Assignee)

Comment 24

8 days ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 days ago
Attachment #8886020 - Flags: review?(cam)
(Assignee)

Comment 27

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

Comment 28

5 days ago
mozreview-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

::: 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+
(Assignee)

Comment 29

4 days ago
mozreview-review-reply
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.
(Assignee)

Comment 30

4 days ago
Servo PR: servo/servo#17764
Comment hidden (mozreview-request)
(Assignee)

Updated

4 days ago
Attachment #8886020 - Attachment is obsolete: true

Comment 32

4 days ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3df984fd0ba2
Update reftest expectation. r=emilio

Comment 33

3 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3df984fd0ba2
Status: NEW → RESOLVED
Last Resolved: 3 days 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.