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)
Core
Layout
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
Assignee | ||
Comment 1•9 years ago
|
||
- 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)
Assignee | ||
Comment 2•9 years ago
|
||
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.
Assignee | ||
Comment 5•9 years ago
|
||
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
Assignee | ||
Comment 6•9 years ago
|
||
(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 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
- 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
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 9•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef3881f8d1a3
Keywords: checkin-needed
Comment 10•9 years ago
|
||
sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8fe7716a118b since one of this 2 changes caused https://treeherder.mozilla.org/logviewer.html#?job_id=7584300&repo=mozilla-inbound as perma failure
Flags: needinfo?(kgilbert)
Comment 11•9 years ago
|
||
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)
Comment 12•9 years ago
|
||
(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)
Comment 13•9 years ago
|
||
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)
Comment 14•9 years ago
|
||
(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.
Comment 15•9 years ago
|
||
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.
Comment 16•9 years ago
|
||
s/abondon/abandon/
Comment 17•9 years ago
|
||
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).
(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.)
Comment 20•9 years ago
|
||
(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.
Assignee | ||
Comment 21•9 years ago
|
||
- 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)
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 22•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cda379df9190
Keywords: checkin-needed
Comment 23•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cda379df9190
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•