Closed Bug 1140623 Opened 9 years ago Closed 9 years ago

Some mochitests fail when the layout.css.scroll-snap.enabled pref is enabled

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: kip, Assigned: kip)

References

Details

Attachments

(1 file, 2 obsolete files)

Some CSS related mochitests failed only when the CSS scroll snapping is enabled (layout.css.scroll-snap.enabled preference):

layout/style/test/test_bug657143.html
layout/style/test/test_compute_data_with_start_struct.html
layout/style/test/test_property_syntax_errors.html
layout/style/test/test_style_struct_copy_constructors.html
layout/style/test/test_value_computation.html

Output from these tests:

*layout/style/test/test_bug657143.html*

27 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_bug657143.html | CSS property list should be alphabetical - got "align-content,align-items,align-self,animation-delay,animation-direction,animation-duration,animation-fill-mode,animation-iteration-count,animation-name,animation-play-state,animation-timing-function,backface-visibility,background-attachment,background-blend-mode,background-clip,background-color,background-image,background-origin,background-position,background-repeat,background-size,border-bottom-color,border-bottom-left-radius,border-bottom-right-radius,border-bottom-style,border-bottom-width,border-collapse,border-image-outset,border-image-repeat,border-image-slice,border-image-source,border-image-width,border-left-color,border-left-style,border-left-width,border-right-color,border-right-style,border-right-width,border-spacing,border-top-color,border-top-left-radius,border-top-right-radius,border-top-style,border-top-width,bottom,box-decoration-break,box-shadow,box-sizing,caption-side,clear,clip,color,content,counter-increment,counter-reset,cursor,direction,display,empty-cells,flex-basis,flex-direction,flex-grow,flex-shrink,flex-wrap,float,font-family,font-feature-settings,font-kerning,font-language-override,font-size,font-size-adjust,font-stretch,font-style,font-synthesis,font-variant,font-variant-alternates,font-variant-caps,font-variant-east-asian,font-variant-ligatures,font-variant-numeric,font-variant-position,font-weight,grid-auto-columns,grid-auto-flow,grid-auto-rows,grid-column-end,grid-column-start,grid-row-end,grid-row-start,grid-template-areas,grid-template-columns,grid-template-rows,height,image-orientation,ime-mode,isolation,justify-content,left,letter-spacing,line-height,list-style-image,list-style-position,list-style-type,margin-bottom,margin-left,margin-right,margin-top,marker-offset,max-height,max-width,min-height,min-width,mix-blend-mode,object-fit,object-position,opacity,order,outline-color,outline-offset,outline-style,outline-width,overflow,overflow-x,overflow-y,padding-bottom,padding-left,padding-right,padding-top,page-break-after,page-break-before,page-break-inside,perspective,perspective-origin,pointer-events,position,quotes,resize,right,ruby-align,ruby-position,scroll-behavior,scroll-snap-type-x,scroll-snap-type-y,scroll-snap-points-x,scroll-snap-points-y,scroll-snap-destination,scroll-snap-coordinate,table-layout,text-align,text-decoration,text-decoration-color,text-decoration-line,text-decoration-style,text-indent,text-overflow,text-shadow,text-transform,top,transform,transform-origin,transform-style,transition-delay,transition-duration,transition-property,transition-timing-function,unicode-bidi,vertical-align,visibility,white-space,width,will-change,word-break,word-spacing,word-wrap,z-index", expected "align-content,align-items,align-self,animation-delay,animation-direction,animation-duration,animation-fill-mode,animation-iteration-count,animation-name,animation-play-state,animation-timing-function,backface-visibility,background-attachment,background-blend-mode,background-clip,background-color,background-image,background-origin,background-position,background-repeat,background-size,border-bottom-color,border-bottom-left-radius,border-bottom-right-radius,border-bottom-style,border-bottom-width,border-collapse,border-image-outset,border-image-repeat,border-image-slice,border-image-source,border-image-width,border-left-color,border-left-style,border-left-width,border-right-color,border-right-style,border-right-width,border-spacing,border-top-color,border-top-left-radius,border-top-right-radius,border-top-style,border-top-width,bottom,box-decoration-break,box-shadow,box-sizing,caption-side,clear,clip,color,content,counter-increment,counter-reset,cursor,direction,display,empty-cells,flex-basis,flex-direction,flex-grow,flex-shrink,flex-wrap,float,font-family,font-feature-settings,font-kerning,font-language-override,font-size,font-size-adjust,font-stretch,font-style,font-synthesis,font-variant,font-variant-alternates,font-variant-caps,font-variant-east-asian,font-variant-ligatures,font-variant-numeric,font-variant-position,font-weight,grid-auto-columns,grid-auto-flow,grid-auto-rows,grid-column-end,grid-column-start,grid-row-end,grid-row-start,grid-template-areas,grid-template-columns,grid-template-rows,height,image-orientation,ime-mode,isolation,justify-content,left,letter-spacing,line-height,list-style-image,list-style-position,list-style-type,margin-bottom,margin-left,margin-right,margin-top,marker-offset,max-height,max-width,min-height,min-width,mix-blend-mode,object-fit,object-position,opacity,order,outline-color,outline-offset,outline-style,outline-width,overflow,overflow-x,overflow-y,padding-bottom,padding-left,padding-right,padding-top,page-break-after,page-break-before,page-break-inside,perspective,perspective-origin,pointer-events,position,quotes,resize,right,ruby-align,ruby-position,scroll-behavior,scroll-snap-coordinate,scroll-snap-destination,scroll-snap-points-x,scroll-snap-points-y,scroll-snap-type-x,scroll-snap-type-y,table-layout,text-align,text-decoration,text-decoration-color,text-decoration-line,text-decoration-style,text-indent,text-overflow,text-shadow,text-transform,top,transform,transform-origin,transform-style,transition-delay,transition-duration,transition-property,transition-timing-function,unicode-bidi,vertical-align,visibility,white-space,width,will-change,word-break,word-spacing,word-wrap,z-index"


*layout/style/test/test_compute_data_with_start_struct.html*

failed | scroll-snap-coordinate is not touched when its value comes from aStartStruct - got none, expected 25% 25%
failed | scroll-snap-destination is not touched when its value comes from aStartStruct - got 0px 0px, expected 25% 25%
failed | scroll-snap-points-x is not touched when its value comes from aStartStruct - got none, expected repeat(100%)
failed | scroll-snap-points-y is not touched when its value comes from aStartStruct - got none, expected repeat(100%)


*layout/style/test/test_property_syntax_errors.html*

failed | empty value 'scroll-snap-points-x:' is balanced and does not lead to parsing errors afterwards - got red, expected green
failed | empty value 'scroll-snap-points-x: ' is balanced and does not lead to parsing errors afterwards - got red, expected green
failed | empty value 'scroll-snap-points-y:' is balanced and does not lead to parsing errors afterwards - got red, expected green
failed | empty value 'scroll-snap-points-y: ' is balanced and does not lead to parsing errors afterwards - got red, expected green


*layout/style/test/test_style_struct_copy_constructors.html*

failed | property 'scroll-snap-points-x' was copy-constructed correctly (aStartStruct) - got none, expected repeat(100%)
failed | property 'scroll-snap-points-y' was copy-constructed correctly (aStartStruct) - got none, expected repeat(100%)
failed | property 'scroll-snap-destination' was copy-constructed correctly (aStartStruct) - got 0px 0px, expected 25% 25%
failed | property 'scroll-snap-coordinate' was copy-constructed correctly (aStartStruct) - got none, expected 25% 25% 


*layout/style/test/test_value_computation.html*

failed | should not get initial value for 'scroll-snap-destination:0 0' on elementn. - didn't expect 0px 0px, but got it
failed | should not get initial value for 'scroll-snap-destination:0 0' on elementf. - didn't expect 0px 0px, but got it
- CSS scroll snapping related attributes are now sorted correctly
- Now passes mochitests with CSS scroll snapping enabled:
- layout/style/test/test_bug657143.html
- layout/style/test/test_compute_data_with_start_struct.html
- layout/style/test/test_property_syntax_errors.html
- layout/style/test/test_style_struct_copy_constructors.html
- layout/style/test/test_value_computation.html
Attachment #8574199 - Flags: review?(cam)
These mochitest failures were found with a try run submitted for Bug 1138651 (Enable CSS Scroll Snapping by Default for B2G):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e04cdef9d8b
The code for scroll-snap-points-{x,y}, scroll-snap-destination, and scroll-snap-coordinate in nsRuleNode::ComputeDisplayData is breaking one of the basic invariants of Compute*Data functions, which is **don't touch anything if the unit is eCSSUnit_Null**.

(Review probably should have caught that.)
I guess you figured that out, though.
I have pushed to try with this patch and Bug 1138651 (Enable CSS Scroll Snapping by Default for B2G) applied, for all B2G builds:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bea1491ba18c
(In reply to :kip (Kearwood Gilbert) from comment #5)
> I have pushed to try with this patch and Bug 1138651 (Enable CSS Scroll
> Snapping by Default for B2G) applied, for all B2G builds:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=bea1491ba18c

Try is now green with the pref enabled for B2G.
Comment on attachment 8574199 [details] [diff] [review]
Bug 1140623 - Correct mochitest failures that occur when the layout.css.scroll-snap.enabled preference is enabled

Review of attachment 8574199 [details] [diff] [review]:
-----------------------------------------------------------------

The reordering in nsCSSPropList.h still has scroll-behaviour in the wrong place (which tests wouldn't fail unless its pref was also enabled).

I don't think any tests rely on the order of the keys on gCSSProperties in property_database.js, but it doesn't hurt to keep them ordered as you've done.

r=me with scroll-behavior in the right place.
Attachment #8574199 - Flags: review?(cam) → review+
- CSS scroll snapping related attributes are now sorted correctly
- Now passes mochitests with CSS scroll snapping enabled:
- layout/style/test/test_bug657143.html
- layout/style/test/test_compute_data_with_start_struct.html
- layout/style/test/test_property_syntax_errors.html
- layout/style/test/test_style_struct_copy_constructors.html
- layout/style/test/test_value_computation.html

V2 Patch:
- Corrected sort order of scroll-behavior in nsCSSPropList.h as per review in Comment 7
Attachment #8574199 - Attachment is obsolete: true
Keywords: checkin-needed
Xidorn, is this just a matter of declaring in generate-stylestructlist.py that Display now depends on Font (due to the new scroll-snap-* properties that can take em-unit values)?
Flags: needinfo?(quanxunzhen)
(In reply to Cameron McCormack (:heycam) from comment #11)
> Xidorn, is this just a matter of declaring in generate-stylestructlist.py
> that Display now depends on Font (due to the new scroll-snap-* properties
> that can take em-unit values)?

Yes, it is a matter. I've just removed the indirect dependency between Display and Visibility because of the writing-mode fixup for inter-character. If Display depends on Font, then the dependency is back again. I wonder if it is possible to put that in Position as well just like other properties moved out from the Display in bug 1139283.

TBH, I really feel uncomfortable to do these hairy things because of inter-character ruby, which is only used in a very limited fields in a very limited area.
Flags: needinfo?(quanxunzhen)
OK, so your cyclic dependency assertions would get triggered if we noted the dependency in generate-stylestructlist.py.

So yeah I guess we should move these properties to Position.

(Although that struct is getting pretty big.  It is a non-inherited struct, so I suppose we won't get that many of them.  Here are the current style struct sizes on 64-bit, FWIW:

Font: 120 bytes
Color: 4 bytes
List: 56 bytes
Text: 80 bytes
Visibility: 6 bytes
Quotes: 16 bytes
UserInterface: 24 bytes
TableBorder: 12 bytes
SVG: 128 bytes
Variables: 48 bytes
Background: 160 bytes
Position: 608 bytes
TextReset: 80 bytes
Display: 280 bytes
Content: 56 bytes
UIReset: 4 bytes
Table: 8 bytes
Margin: 64 bytes
Padding: 64 bytes
Border: 312 bytes
Outline: 112 bytes
XUL: 16 bytes
SVGReset: 64 bytes
Column: 56 bytes)
(In reply to Cameron McCormack (:heycam) from comment #13)
> OK, so your cyclic dependency assertions would get triggered if we noted the
> dependency in generate-stylestructlist.py.

No, it won't trigger, because it is not declared in that file, but is fixed up in nsStyleContext::ApplyStyleFixups.

> So yeah I guess we should move these properties to Position.
> 
> (Although that struct is getting pretty big.  It is a non-inherited struct,
> so I suppose we won't get that many of them.  Here are the current style
> struct sizes on 64-bit, FWIW:
> 
> Font: 120 bytes
> Color: 4 bytes
> List: 56 bytes
> Text: 80 bytes
> Visibility: 6 bytes
> Quotes: 16 bytes
> UserInterface: 24 bytes
> TableBorder: 12 bytes
> SVG: 128 bytes
> Variables: 48 bytes
> Background: 160 bytes
> Position: 608 bytes
> TextReset: 80 bytes
> Display: 280 bytes
> Content: 56 bytes
> UIReset: 4 bytes
> Table: 8 bytes
> Margin: 64 bytes
> Padding: 64 bytes
> Border: 312 bytes
> Outline: 112 bytes
> XUL: 16 bytes
> SVGReset: 64 bytes
> Column: 56 bytes)

So I guess it's now probably a good time to split that struct? I think most of pages would only use x/y/width/height, so maybe we should move them somewhere else.
Actually, given the writing mode spec and the ruby spec, there always exists cyclic dependencies between writing-mode and display. If we abondon inter-character ruby, though, the cyclic will be removed, then.
s/abondon/abandon/
Xidorn clarified for me on IRC that this will be a problem (maybe) only once he adds the inter-character ruby style fixups, and that for the moment it is not a problem to have Display depend on Font.

So, Kip: you just need to add the dependency declaration in generate-stylestructlist.py (i.e. add " + LENGTH_DEP" on the line that declares Display).
See Also: → 1139283
(In reply to Cameron McCormack (:heycam) from comment #17)
> Xidorn clarified for me on IRC that this will be a problem (maybe) only once
> he adds the inter-character ruby style fixups, and that for the moment it is
> not a problem to have Display depend on Font.

Though he also just did a bunch of work to fix all the other cases of that problem -- although it's still not entirely clear that the whole thing will work out, so maybe it's not worth doing more?
(It also might make sense to move them to nsStyleUIReset.)
(In reply to David Baron [:dbaron] (UTC-7) from comment #18)
> Though he also just did a bunch of work to fix all the other cases of that
> problem -- although it's still not entirely clear that the whole thing will
> work out, so maybe it's not worth doing more?

Yeah, though since the properties are already on Display, I don't think there's a need to delay enabling scroll snapping on B2G by moving those properties right now.
- CSS scroll snapping related attributes are now sorted correctly
- Now passes mochitests with CSS scroll snapping enabled:
- layout/style/test/test_bug657143.html
- layout/style/test/test_compute_data_with_start_struct.html
- layout/style/test/test_property_syntax_errors.html
- layout/style/test/test_style_struct_copy_constructors.html
- layout/style/test/test_value_computation.html

V3 Patch:

Added dependency declaration in generate-stylestructlist.py, as per Comment 17
Attachment #8576794 - Attachment is obsolete: true
Flags: needinfo?(kgilbert)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cda379df9190
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.