Closed Bug 1345498 Opened 7 years ago Closed 7 years ago

stylo: implement/parse text-justify and its gecko glue

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: chenpighead, Assigned: chenpighead)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1345498 - [stylo] update mochitest expections for text-justify.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
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.
Attachment #8844951 - Flags: review?(xidorn+moz)
Attachment #8844952 - Flags: review?(xidorn+moz)
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 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 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 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+
(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.
Attachment #8844952 - Attachment is obsolete: true
PR has been merged to Servo and autoland. Let's land the mochitest part.
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b17b5fdb4ae7
[stylo] update mochitest expections for text-justify. r=xidorn
(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.
https://hg.mozilla.org/mozilla-central/rev/b17b5fdb4ae7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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)
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)
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...
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)
This pretty much looks like a bug of MozReview. At least its interdiff looks very inconsistent on commit message.
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.]
See Also: → 1346321
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: