Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only count outline width in overflow calculation if outline style is active. #6251

Merged
merged 1 commit into from Jun 4, 2015

Conversation

@glennw
Copy link
Member

glennw commented Jun 2, 2015

This fixes layers being created with a 3x3 overflow that's not needed.

This exposes #6250, so update the affected reftest to use ahem font until it is fixed.

Review on Reviewable

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Jun 2, 2015

Critic review: https://critic.hoppipolla.co.uk/r/5154

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@glennw
Copy link
Member Author

glennw commented Jun 2, 2015

@nox
Copy link
Member

nox commented Jun 2, 2015

Review status: all files reviewed, 1 unresolved discussion, all commit checks successful.
Reviewed files:

  • components/layout/fragment.rs @ r1
  • tests/ref/inline_block_stacking_context_a.html @ r1
  • tests/ref/inline_block_stacking_context_ref.html @ r1

components/layout/fragment.rs, line 2049 [r1] (raw file):
This doesn't seem to be the right place to check for this, the spec says that the outline width's computed value must be 0 if the outline style is none.

My guess is that we should introduce a new type OutlineWidth which computes to 0 in its ToComputedValue implementation if outline-style is none in the context.


Comments from the review on Reviewable.io

@SimonSapin
Copy link
Member

SimonSapin commented Jun 2, 2015

Review status: all files reviewed, 1 unresolved discussion, all commit checks successful.


components/layout/fragment.rs, line 2049 [r1] (raw file):
Right, this should be done in a ToComputedValue impl like the one for border-*-width.


Comments from the review on Reviewable.io

@SimonSapin
Copy link
Member

SimonSapin commented Jun 3, 2015

Review status: 4 of 5 files reviewed, 1 unresolved discussion, all commit checks successful.
Reviewed files:

  • components/style/properties.mako.rs @ r2
  • components/style/values.rs @ r2

Comments from the review on Reviewable.io

@nox
Copy link
Member

nox commented Jun 3, 2015

Review status: all files reviewed, 2 unresolved discussions, all commit checks successful.
Reviewed files:

  • components/layout/fragment.rs @ r2
  • components/style/properties.mako.rs @ r2

components/style/properties.mako.rs, line 4525 [r2] (raw file):
This should use properties::longhands::outline_width::parse (see for example how "background" is handled in this same file).


components/style/properties.mako.rs, line 4537 [r2] (raw file):
As mentioned above, change the parse routine instead.


Comments from the review on Reviewable.io

@nox
Copy link
Member

nox commented Jun 3, 2015

Feel free to squash everything while at it when you address my two last remarks.

On a more general note, if you know that the reviewer will use Reviewable, it's generally safe to just rebase and squash as you wish.

@frewsxcv
Copy link
Member

frewsxcv commented Jun 3, 2015

Review status: all files reviewed, 3 unresolved discussions, all commit checks successful.


components/style/properties.mako.rs, line 5600 [r2] (raw file):
This doesn't get assigned to anything; what's the point of the match?


Comments from the review on Reviewable.io

@frewsxcv
Copy link
Member

frewsxcv commented Jun 3, 2015

Review status: all files reviewed, 2 unresolved discussions, all commit checks successful.


components/style/properties.mako.rs, line 5600 [r2] (raw file):
Oh nevermind, the lack of indentation confused me. It would be nice if it was indented so that it is obvious there is something above it.


Comments from the review on Reviewable.io

@nox
Copy link
Member

nox commented Jun 3, 2015

Review status: all files reviewed, 3 unresolved discussions, all commit checks successful.
Reviewed files:

  • components/style/values.rs @ r2

components/style/properties.mako.rs, line 5603 [r2] (raw file):
Indentation.


Comments from the review on Reviewable.io

@glennw
Copy link
Member Author

glennw commented Jun 4, 2015

Review status: 3 of 4 files reviewed, 3 unresolved discussions, all commit checks successful.


components/style/properties.mako.rs, line 4525 [r2] (raw file):
Done.


components/style/properties.mako.rs, line 4537 [r2] (raw file):
Done.


components/style/properties.mako.rs, line 5603 [r2] (raw file):
Was copy pasted from the block below - updated to fix both blocks.


Comments from the review on Reviewable.io

…active.

This fixes layers being created with a 3x3 border that's not needed.

This exposes #6250, so update the affected reftest to use ahem font until it is fixed.
@glennw glennw force-pushed the glennw:fix-overflow branch from a8ed3d0 to 958adc0 Jun 4, 2015
@nox
Copy link
Member

nox commented Jun 4, 2015

@bors-servo: r+

-S-awaiting-review +S-awaiting-merge


Review status: :shipit: all files reviewed, all discussions resolved, all commit checks successful.
Reviewed files:

  • components/style/properties.mako.rs @ r3

Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2015

📌 Commit 958adc0 has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2015

Testing commit 958adc0 with merge 610de77...

bors-servo pushed a commit that referenced this pull request Jun 4, 2015
This fixes layers being created with a 3x3 overflow that's not needed.

This exposes #6250, so update the affected reftest to use ahem font until it is fixed.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6251)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2015

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2

@bors-servo bors-servo merged commit 958adc0 into servo:master Jun 4, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@glennw glennw deleted the glennw:fix-overflow branch Jul 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.