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

Implement 'align' attribute for <div> elements #7953

Merged
merged 1 commit into from Oct 10, 2015

Conversation

@frewsxcv
Copy link
Member

frewsxcv commented Oct 10, 2015

Review on Reviewable

@highfive
Copy link

highfive commented Oct 10, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 10, 2015

So I need a little help with this.

At the time of writing, this test fails after this pull request:
tests/wpt/metadata/html/rendering/non-replaced-elements/flow-content-0/div-align.html

I'm actually not even sure how it was passing prior to this getting implemented. I'm guessing it fails now because this isn't implemented? If anyone has any thoughts, please let me know, thanks.

@eefriedman
Copy link
Contributor

eefriedman commented Oct 10, 2015

See

div[align=left i] { text-align: -servo-left; }
. See also #7825.

I don't think there's any reason to move the rendering rule from presentational-hints.css to synthesize_presentational_hints_for_legacy_attributes.

@nox nox closed this Oct 10, 2015
@nox nox reopened this Oct 10, 2015
@nox
Copy link
Member

nox commented Oct 10, 2015

I don't think there's any reason to move the rendering rule from presentational-hints.css to synthesize_presentational_hints_for_legacy_attributes.

Not all presentational hints can be expressed through CSS, though.

@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 10, 2015

Will those CSS hints work correctly with the IDL/content getters/setters? Also, align=center is not part of the spec

@nox
Copy link
Member

nox commented Oct 10, 2015

@frewsxcv You sure?

The center element, and the div element when it has an align attribute whose value is an ASCII case-insensitive match for either the string "center" or the string "middle", are expected to center text within themselves, as if they had their 'text-align' property set to 'center' in a presentational hint, and to align descendants to the center.

@nox
Copy link
Member

nox commented Oct 10, 2015

@frewsxcv I'm pretty sure you can just implement reflecting IDL getters and setters and consider that attribute to hold onto a plain old string.

@nox
Copy link
Member

nox commented Oct 10, 2015

@frewsxcv For other presentational hints, like the one related to the non-zero table border, or fancy color attributes that need to be parsed, we can't avoid going through the synthesis machinery, but I'm pretty sure we prefer going through CSS for the simpler one, as mentioned by @eefriedman.

@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 10, 2015

You're right. I totally glossed over that first paragraph because I thought it only applied to center elements

@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 10, 2015

Thanks all for the help, very helpful information to know :)

Force push incoming...

@frewsxcv frewsxcv force-pushed the frewsxcv:htmldivelement-align branch from 96f8612 to 55c65d3 Oct 10, 2015
@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 10, 2015

Ready for review

@mbrubeck mbrubeck self-assigned this Oct 10, 2015
@mbrubeck
Copy link
Contributor

mbrubeck commented Oct 10, 2015

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from the review on Reviewable.io

@mbrubeck
Copy link
Contributor

mbrubeck commented Oct 10, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 10, 2015

📌 Commit 55c65d3 has been approved by mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented Oct 10, 2015

Testing commit 55c65d3 with merge 0dd6556...

bors-servo pushed a commit that referenced this pull request Oct 10, 2015
Implement 'align' attribute for <div> elements



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

bors-servo commented Oct 10, 2015

💔 Test failed - mac-rel-wpt

@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 10, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 10, 2015

Testing commit 55c65d3 with merge babeed9...

bors-servo pushed a commit that referenced this pull request Oct 10, 2015
Implement 'align' attribute for <div> elements



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

bors-servo commented Oct 10, 2015

@bors-servo bors-servo merged commit 55c65d3 into servo:master Oct 10, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@frewsxcv frewsxcv deleted the frewsxcv:htmldivelement-align branch Oct 10, 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

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