style: Dishonor display: -moz-box if -webkit-box was specified before #18867

emilio · 2017-10-13T14:54:35Z

This is a compatibility hack that Gecko supports that is apparently important for android.

I want to remove it, but let's see...

See https://bugzilla.mozilla.org/show_bug.cgi?id=1407701 for reference.

This change is

highfive · 2017-10-13T14:54:40Z

Heads up! This PR modifies the following files:

@bholley: components/style/stylesheets/keyframes_rule.rs, components/style/properties/longhand/box.mako.rs, components/style/properties/declaration_block.rs, ports/geckolib/glue.rs
@canaltinova: components/style/stylesheets/keyframes_rule.rs, components/style/properties/longhand/box.mako.rs, components/style/properties/declaration_block.rs
@fitzgen: components/script/dom/cssstyledeclaration.rs
@KiChjang: components/script/dom/cssstyledeclaration.rs

highfive · 2017-10-13T14:54:41Z

Warning

These commits modify style and script code, but no tests are modified. Please consider adding a test!

emilio · 2017-10-13T14:55:04Z

r? @SimonSapin or @heycam

jdm · 2017-10-13T15:39:47Z

error: unused variable: `old_display`
    --> C:\projects\servo\target\debug\build\style-18845a8bb7b22603\out/properties.rs:6663:17
     |
6663 |                 old_display: Self,
     |                 ^^^^^^^^^^^
     |
note: lint level defined here
    --> C:\projects\servo\components\style\lib.rs:26:9
     |
26   | #![deny(warnings)]
     |         ^^^^^^^^
     = note: #[deny(unused_variables)] implied by #[deny(warnings)]
     = note: to avoid this warning, consider using `_old_display` instead
error: unused variable: `new_display`
    --> C:\projects\servo\target\debug\build\style-18845a8bb7b22603\out/properties.rs:6664:17
     |
6664 |                 new_display: Self,
     |                 ^^^^^^^^^^^
     |
     = note: to avoid this warning, consider using `_new_display` instead
error: aborting due to 2 previous errors
error: Could not compile `style`.
Caused by:

heycam · 2017-10-14T01:48:57Z

@bors-servo delegate+

(I reviewed this in Bugzilla.)

bors-servo · 2017-10-14T01:48:58Z

✌️ @emilio can now approve this pull request

jdm · 2017-10-14T02:51:26Z

error[E0061]: this function takes 3 parameters but 2 parameters were supplied

  --> /home/travis/build/servo/servo/tests/unit/style/keyframes.rs:77:17

   |

77 | /                 PropertyDeclaration::Width(

78 | |                     LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32))),

   | |___________________________________________________________________________________^ expected 3 parameters

error[E0061]: this function takes 3 parameters but 2 parameters were supplied

  --> /home/travis/build/servo/servo/tests/unit/style/keyframes.rs:82:17

   |

82 | /                 PropertyDeclaration::Height(

83 | |                     LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32))),

   | |___________________________________________________________________________________^ expected 3 parameters

error[E0061]: this function takes 3 parameters but 2 parameters were supplied

   --> /home/travis/build/servo/servo/tests/unit/style/keyframes.rs:133:17

    |

133 | /                 PropertyDeclaration::Width(

134 | |                     LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32))),

    | |___________________________________________________________________________________^ expected 3 parameters

error[E0061]: this function takes 3 parameters but 2 parameters were supplied

   --> /home/travis/build/servo/servo/tests/unit/style/keyframes.rs:138:17

    |

138 | /                 PropertyDeclaration::Height(

139 | |                     LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32))),

    | |___________________________________________________________________________________^ expected 3 parameters

error[E0061]: this function takes 3 parameters but 2 parameters were supplied

   --> /home/travis/build/servo/servo/tests/unit/style/keyframes.rs:196:17

    |

196 | /                 PropertyDeclaration::Width(

197 | |                     LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32))),

    | |___________________________________________________________________________________^ expected 3 parameters

error[E0061]: this function takes 3 parameters but 2 parameters were supplied

   --> /home/travis/build/servo/servo/tests/unit/style/keyframes.rs:201:17

    |

201 | /                 PropertyDeclaration::Height(

202 | |                     LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32))),

    | |___________________________________________________________________________________^ expected 3 parameters

   Compiling metrics_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/metrics)

error[E0061]: this function takes 3 parameters but 2 parameters were supplied

  --> /home/travis/build/servo/servo/tests/unit/style/stylesheets.rs:38:20

   |

38 |         block.push(d, i)

   |                    ^^^^ expected 3 parameters

error[E0308]: mismatched types

  --> /home/travis/build/servo/servo/tests/unit/style/stylesheets.rs:38:9

   |

38 |         block.push(d, i)

   |         ^^^^^^^^^^^^^^^^- help: try adding a semicolon: `;`

   |         |

   |         expected (), found bool

   |

   = note: expected type `()`

              found type `bool`

error: aborting due to 8 previous errors

error: Could not compile `style_tests`.

This introduces DeclarationSource, to see if a declaration has been parsed or set from CSSOM in a declaration block. The Servo_DeclarationBlock_SetFoo and similar callers are changed to DeclarationSource::CssOm because their semantics are more CSSOM-y, but it shouldn't matter since they should all be checked before hand with Servo_DeclarationBlock_PropertyIsSet.

Bug: 1407701

emilio · 2017-10-14T09:32:44Z

@bors-servo r=heycam

bors-servo · 2017-10-14T09:32:44Z

📌 Commit 6d694a4 has been approved by heycam

bors-servo · 2017-10-14T10:27:55Z

⌛ Testing commit 6d694a4 with merge 2be76c5...

style: Dishonor display: -moz-box if -webkit-box was specified before This is a compatibility hack that Gecko supports that is apparently important for android. I want to remove it, but let's see... See https://bugzilla.mozilla.org/show_bug.cgi?id=1407701 for reference.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18867)

bors-servo · 2017-10-14T11:58:25Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: heycam
Pushing 2be76c5 to master...

The declaration can indeed have the same id if it contains variables, or is a CSS keyword value. This was making a WPT test fail, so this is tested, though Servo's WPT import didn't catch this for some reason.

style: Fixup the hack added in #18867. The declaration can indeed have the same id if it contains variables, or is a CSS keyword value. This was making a WPT test fail, so this is tested, though Servo's WPT import didn't catch this for some reason.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18880)

The declaration can indeed have the same id if it contains variables, or is a CSS keyword value. This was making a WPT test fail, so this is tested, though Servo's WPT import didn't catch this for some reason.

highfive assigned glennw Oct 13, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 13, 2017

highfive assigned SimonSapin and unassigned glennw Oct 13, 2017

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 13, 2017

emilio force-pushed the parse-hack branch from 81a8a83 to d1f1b92 Compare October 13, 2017 15:46

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Oct 13, 2017

emilio added 2 commits October 14, 2017 11:30

style: Dishonor display: -moz-box if -webkit-box was specified before.

6d694a4

Bug: 1407701

emilio force-pushed the parse-hack branch from d1f1b92 to 6d694a4 Compare October 14, 2017 09:31

highfive assigned heycam and unassigned SimonSapin Oct 14, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 14, 2017

bors-servo merged commit 6d694a4 into servo:master Oct 14, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style: Dishonor display: -moz-box if -webkit-box was specified before #18867

style: Dishonor display: -moz-box if -webkit-box was specified before #18867

emilio commented Oct 13, 2017 •

edited by SimonSapin

highfive commented Oct 13, 2017

highfive commented Oct 13, 2017

emilio commented Oct 13, 2017

jdm commented Oct 13, 2017

heycam commented Oct 14, 2017

bors-servo commented Oct 14, 2017

jdm commented Oct 14, 2017

emilio commented Oct 14, 2017

bors-servo commented Oct 14, 2017

bors-servo commented Oct 14, 2017

bors-servo commented Oct 14, 2017

style: Dishonor display: -moz-box if -webkit-box was specified before #18867

style: Dishonor display: -moz-box if -webkit-box was specified before #18867

Conversation

emilio commented Oct 13, 2017 • edited by SimonSapin

highfive commented Oct 13, 2017

highfive commented Oct 13, 2017

emilio commented Oct 13, 2017

jdm commented Oct 13, 2017

heycam commented Oct 14, 2017

bors-servo commented Oct 14, 2017

jdm commented Oct 14, 2017

emilio commented Oct 14, 2017

bors-servo commented Oct 14, 2017

bors-servo commented Oct 14, 2017

bors-servo commented Oct 14, 2017

emilio commented Oct 13, 2017 •

edited by SimonSapin