Closed
Bug 1345498
Opened 8 years ago
Closed 8 years ago
stylo: implement/parse text-justify and its gecko glue
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: chenpighead, Assigned: chenpighead)
References
Details
Attachments
(1 file, 1 obsolete file)
In this bug, I intent to implement the missing part of text-justify property for stylo. I assume that the mochitest expectations that I've added in Bug 1343593 could be removed after this.
Not sure if I should block stylo or stylo-bustage, feel free to correct me.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8844951 -
Flags: review?(xidorn+moz)
Attachment #8844952 -
Flags: review?(xidorn+moz)
Comment 4•8 years ago
|
||
mozreview-review |
Comment on attachment 8844951 [details]
Bug 1345498 - [stylo] update mochitest expections for text-justify.
https://reviewboard.mozilla.org/r/118206/#review120268
Too complicated. Something like https://github.com/servo/servo/blob/5fe921f2ab81726dc34b0c427580f355d503f56e/components/style/properties/longhand/inherited_box.mako.rs#L29-L64 should be enough.
Attachment #8844951 -
Flags: review?(xidorn+moz) → review-
Comment 5•8 years ago
|
||
mozreview-review |
Comment on attachment 8844952 [details]
Bug 1345498 - [stylo] update mochitest expections for text-justify.
https://reviewboard.mozilla.org/r/118208/#review120270
Attachment #8844952 -
Flags: review?(xidorn+moz) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8844951 [details]
Bug 1345498 - [stylo] update mochitest expections for text-justify.
https://reviewboard.mozilla.org/r/118206/#review120386
::: servo/components/style/properties/gecko.mako.rs:2610
(Diff revision 2)
> </%self:impl_trait>
>
>
> <%self:impl_trait style_struct_name="InheritedText"
> skip_longhands="text-align text-emphasis-style text-shadow line-height letter-spacing word-spacing
> - -webkit-text-stroke-width text-emphasis-position -moz-tab-size">
> + -webkit-text-stroke-width text-emphasis-position -moz-tab-size text-justify">
I don't think you need this, neither the code you added below in the same file...
I guess you can simply add `gecko_enum_prefix` to the block in inherited_text.mako.rs, and everything should just work...
::: servo/components/style/properties/longhand/inherited_text.mako.rs:196
(Diff revision 2)
> animatable=False,
> spec="https://drafts.csswg.org/css-text/#propdef-word-break")}
>
> -// TODO(pcwalton): Support `text-justify: distribute`.
> -${helpers.single_keyword("text-justify",
> - "auto none inter-word",
> +// TODO(pcwalton): Support `text-justify: distribute`
> +<%helpers:single_keyword_computed name="text-justify"
> + values="auto none inter-word inter-character"
Servo doesn't support `inter-character`, so you should probably move it into `extra_gecko_values`, and make the corresponding branch in `to_computed_value` only generate when `product` is `gecko`.
Attachment #8844951 -
Flags: review?(xidorn+moz)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8844951 [details]
Bug 1345498 - [stylo] update mochitest expections for text-justify.
https://reviewboard.mozilla.org/r/118206/#review120470
Remember that you cannot land Servo code to autoland directly. You need to open a GitHub pull request for that, and r? @upsuper there.
Attachment #8844951 -
Flags: review?(xidorn+moz) → review+
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #11)
> Comment on attachment 8844951 [details]
> Bug 1345498 - [stylo] gecko glue code for text-justify.
>
> https://reviewboard.mozilla.org/r/118206/#review120470
>
> Remember that you cannot land Servo code to autoland directly. You need to
> open a GitHub pull request for that, and r? @upsuper there.
Will do. And I suppose I should only push mochitest update part once my PR is merged to Servo, and then merged to m-c.
Thank you for the review.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8844952 -
Attachment is obsolete: true
Assignee | ||
Comment 16•8 years ago
|
||
PR has been merged to Servo and autoland. Let's land the mochitest part.
Comment 17•8 years ago
|
||
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b17b5fdb4ae7
[stylo] update mochitest expections for text-justify. r=xidorn
Assignee | ||
Comment 18•8 years ago
|
||
(In reply to Pulsebot from comment #17)
> Pushed by jichen@mozilla.com:
> https://hg.mozilla.org/integration/autoland/rev/b17b5fdb4ae7
> [stylo] update mochitest expections for text-justify. r=xidorn
The the commit message seems showing the wrong authorship. It's a git-bz-moz bug: https://github.com/mozilla/git-bz-moz/issues/90
Since this is just something unrelated to the patch itself, I think we should be fine. Will double check next time.
Comment 19•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 20•8 years ago
|
||
Can you remove the following file?
new file mode 100644
--- /dev/null
+++ b/commit-message-875d6
@@ -0,0 +1,3 @@
+Bug 1345498 - [stylo] update mochitest expections for text-justify.
+
+MozReview-Commit-ID: 3A97wLlh1Oy
Flags: needinfo?(jeremychen)
Comment 21•8 years ago
|
||
That's not a real file that lands as part of the commit -- it's a virtual file created in mozreview, for the purpose of letting reviewers leave feedback on the commit message. It doesn't exist in the final landed patch.
(This is a new thing as of ~1 week ago.)
Flags: needinfo?(jeremychen)
Comment 22•8 years ago
|
||
Er, wait, my bad... the commit message file *did* actually land as part of the cset linked in comment 19!
It's not supposed to, when stuff lands via autoland. Maybe that's a bug in this new MozReview feature...
Comment 23•8 years ago
|
||
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30e3c78a7ebd
followup: remove bogus 'commit-message-875d6' file that was accidentally landed as part of this bug's patch. (no review, NPOTB, DONTBUILD)
Comment 24•8 years ago
|
||
This pretty much looks like a bug of MozReview. At least its interdiff looks very inconsistent on commit message.
Comment 25•8 years ago
|
||
Filed bug 1346443.
Comment 26•8 years ago
|
||
Yeah, sorry -- I filed that MozReview commit-message-* thing earlier, as bug 1346321, and forgot to add a link to it here. [duped as-appropriate.]
Comment 27•8 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•