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

Allow retrieving width/height for non-positioned elements #8202

Merged
merged 1 commit into from Nov 4, 2015

Conversation

@dzbarsky
Copy link
Member

dzbarsky commented Oct 26, 2015

This was causing a bunch of tests in tests/wpt/web-platform-tests/html/semantics/embedded-content/the-canvas-element/size.attributes* to fail. They were returning "auto" instead of the correct size. They still fail because the returned size is off by a few pixels, not sure why yet. But this is more correct and may fix other failing tests.

Review on Reviewable

@highfive
Copy link

highfive commented Oct 26, 2015

warning Warning warning

  • These commits modify layout code, but no reftests are modified. Please consider adding a reftest!
@dzbarsky
Copy link
Member Author

dzbarsky commented Oct 26, 2015

Ah the reason for the difference is http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/common/canvas-tests.css#72 which causes the canvas to end up with a border of 2px, so http://mxr.mozilla.org/servo/source/components/layout/query.rs#229 thinks we have a width/height of 4px each.

@pcwalton any ideas what the right thing to do here is?

@bors-servo
Copy link
Contributor

bors-servo commented Oct 26, 2015

The latest upstream changes (presumably #8178) made this pull request unmergeable. Please resolve the merge conflicts.

@eefriedman
Copy link
Contributor

eefriedman commented Oct 26, 2015

@dzbarsky From https://drafts.csswg.org/cssom/#resolved-values : "If the property applies to the element or pseudo-element and the resolved value of the display property is not none, the resolved value is the used value. Otherwise the resolved value is the computed value."

@dzbarsky
Copy link
Member Author

dzbarsky commented Oct 26, 2015

I know, that's why I made this change. Note that it only says things should be positioned for left/right/top/bottom. Is there something I'm missing? (Note that I wrote that code originally, but I hadn't read the spec carefully enough)

@eefriedman
Copy link
Contributor

eefriedman commented Oct 26, 2015

Oh, sorry, I got confused for a moment by the "display: none" bit.

It looks like PositionRetrievingFragmentBorderBoxIterator returns the width as if we were in box-sizing: border-box mode; you might need to change it to explicitly subtract out the border.

@eefriedman
Copy link
Contributor

eefriedman commented Oct 27, 2015

Ah, found it: you want the stacking_relative_content_box method of Fragment.

@dzbarsky
Copy link
Member Author

dzbarsky commented Oct 27, 2015

Ah, thanks for the pointer. That did the trick! Let's see how many tests pass now...

@dzbarsky dzbarsky force-pushed the dzbarsky:getComputedStyle branch from fa8cd40 to 572667e Oct 27, 2015
@glennw
Copy link
Member

glennw commented Oct 27, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 27, 2015

🔒 Merge conflict

@dzbarsky dzbarsky force-pushed the dzbarsky:getComputedStyle branch from 572667e to dee725b Oct 27, 2015
@glennw
Copy link
Member

glennw commented Oct 27, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 27, 2015

Trying commit dee725b with merge e1e5a4e...

bors-servo added a commit that referenced this pull request Oct 27, 2015
Allow retrieving width/height for non-positioned elements

This was causing a bunch of tests in tests/wpt/web-platform-tests/html/semantics/embedded-content/the-canvas-element/size.attributes* to fail.  They were returning "auto" instead of the correct size. They still fail because the returned size is off by a few pixels, not sure why yet. But this is more correct and may fix other failing tests.

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

bors-servo commented Oct 27, 2015

💔 Test failed - linux-rel

@dzbarsky dzbarsky force-pushed the dzbarsky:getComputedStyle branch from dee725b to 2c40ad6 Oct 27, 2015
@jdm jdm removed the S-needs-rebase label Oct 27, 2015
@dzbarsky
Copy link
Member Author

dzbarsky commented Nov 2, 2015

@pcwalton r? (feel free to redirect, but I figured you were a good victim)

@bors-servo
Copy link
Contributor

bors-servo commented Nov 2, 2015

The latest upstream changes (presumably #8245) made this pull request unmergeable. Please resolve the merge conflicts.

match layout_data.data.flow_construction_result {
ConstructionResult::Flow(ref flow_ref, _) =>
flow::base(flow_ref.deref()).stacking_relative_position,
// TODO search parents until we find node with a flow ref.

This comment has been minimized.

Copy link
@pcwalton

pcwalton Nov 2, 2015

Contributor

Style in layout is to say TODO(dzbarsky):

This comment has been minimized.

Copy link
@jdm

jdm Nov 2, 2015

Member

Even better is to file an issue and reference that :)

@jdm
Copy link
Member

jdm commented Nov 3, 2015


/_mozilla/mozilla/getComputedStyle.html
---------------------------------------
FAIL Element's resolved values
FAIL getComputedStyle work for elements not in a document

/cssom-1_dev/html/computed-style-001.htm
----------------------------------------
PASS expected FAIL relative_property_values
@dzbarsky
Copy link
Member Author

dzbarsky commented Nov 4, 2015

first gCS failure was an incorrect test, the 2nd one is a legit failure. But I think we can address that in a followup, this bug is much likelier to affect pages than gCS for elements not in a document.

@dzbarsky dzbarsky force-pushed the dzbarsky:getComputedStyle branch from cf60d85 to d95ca55 Nov 4, 2015
@dzbarsky
Copy link
Member Author

dzbarsky commented Nov 4, 2015

Looks like that didn't work before this PR either, I just fixed the test to test the right thing so it fails now.

@frewsxcv
Copy link
Member

frewsxcv commented Nov 4, 2015

@bors-servo r=pcwalton

@bors-servo
Copy link
Contributor

bors-servo commented Nov 4, 2015

📌 Commit d95ca55 has been approved by pcwalton

@bors-servo
Copy link
Contributor

bors-servo commented Nov 4, 2015

Testing commit d95ca55 with merge acd58be...

bors-servo added a commit that referenced this pull request Nov 4, 2015
Allow retrieving width/height for non-positioned elements

This was causing a bunch of tests in tests/wpt/web-platform-tests/html/semantics/embedded-content/the-canvas-element/size.attributes* to fail.  They were returning "auto" instead of the correct size. They still fail because the returned size is off by a few pixels, not sure why yet. But this is more correct and may fix other failing tests.

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

bors-servo commented Nov 4, 2015

💔 Test failed - mac-rel-wpt

@jdm
Copy link
Member

jdm commented Nov 4, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Nov 4, 2015

Previous build results for android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css are reusable. Rebuilding only mac-rel-wpt...

@bors-servo
Copy link
Contributor

bors-servo commented Nov 4, 2015

💔 Test failed - mac-rel-wpt

@jdm
Copy link
Member

jdm commented Nov 4, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Nov 4, 2015

Testing commit d95ca55 with merge b685085...

bors-servo added a commit that referenced this pull request Nov 4, 2015
Allow retrieving width/height for non-positioned elements

This was causing a bunch of tests in tests/wpt/web-platform-tests/html/semantics/embedded-content/the-canvas-element/size.attributes* to fail.  They were returning "auto" instead of the correct size. They still fail because the returned size is off by a few pixels, not sure why yet. But this is more correct and may fix other failing tests.

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

bors-servo commented Nov 4, 2015

@bors-servo bors-servo merged commit d95ca55 into servo:master Nov 4, 2015
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable Review in progress: 0 of 28 files reviewed, 1 unresolved discussion
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
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

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