1447712 - change computed value of 'letter-spacing: normal' to be 0 internally, with getComputedStyle quirk

Status: RESOLVED DUPLICATE of bug 1532122

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We should change the computed value of 'letter-spacing: normal' to be 0 internally, with getComputedStyle quirk that produces 'normal' for any '0' value, like other browsers do, per:
https://github.com/w3c/csswg-drafts/issues/1484#issuecomment-375015770

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

This should be quite easy to fix.

To fix this, in the Rust side:
* change LetterSpacing in values::specified::text[1] to a struct wrapping Spacing<Length>
* change LetterSpacing in values::computed::text[2] to a struct wrapping Length
* write implementation of ToComputedValue for the specified LetterSpacing to convert normal to zero
* write implementation of ToCss for the computed LetterSpacing to output normal for zero

Then in C++ side:
* make nsComputedDOMStyle::DoGetLetterSpacing[3] output keyword normal for zero value
* change all references of nsStyleText::mLetterSpacing[4] to not check the unit
* change the comment of the above variable[5] to not including normal

And finally write a testharness.js test[6] (if there isn't any) and put it into testing/web-platform/tests/css/css-text/letter-spacing.


[1] https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/servo/components/style/values/specified/text.rs#31
[2] https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/servo/components/style/values/computed/text.rs#26
[3] https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/layout/style/nsComputedDOMStyle.cpp#4732-4738
[4] https://searchfox.org/mozilla-central/search?q=nsStyleText%3A%3AmLetterSpacing&case=false&regexp=false&path=
[5] https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/layout/style/nsStyleStruct.h#1886
[6] http://web-platform-tests.org/writing-tests/testharness.html

Comment 2

[Jet Villegas (inactive)]

[Triage 2018/03/23 - P3]

Comment 3

[Issei Horie [:is2ei]]

Hi, I'm Issei. I would like to work on this issue.
Could you assign this to me?

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Yes, sure. Thanks for working on this! If you have any question, feel free to ask here.

Comment 5

[Issei Horie [:is2ei]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)
> Yes, sure. Thanks for working on this! If you have any question, feel free
> to ask here.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)
> Yes, sure. Thanks for working on this! If you have any question, feel free
> to ask here.

Thanks. Could I ask you a question?

I tried to check the build using ./mach check
Then, I got this error 

=============================================
$ ./mach check
Traceback (most recent call last):
  File "./mach", line 93, in <module>
    main(sys.argv)
  File "./mach", line 23, in main
    mach = mach_bootstrap.bootstrap(topdir)
  File "/Users/horie/oss/mozilla-unified/servo/python/mach_bootstrap.py", line 268, in bootstrap
    _activate_virtualenv(topdir, is_firefox)
  File "/Users/horie/oss/mozilla-unified/servo/python/mach_bootstrap.py", line 217, in _activate_virtualenv
    _process_exec([pip, "install", "-I", "-r", req_path])
  File "/Users/horie/oss/mozilla-unified/servo/python/mach_bootstrap.py", line 109, in _process_exec
    process = Popen(args, stdout=out, stderr=err)
  File "/Users/horie/.pyenv/versions/2.7.11/lib/python2.7/subprocess.py", line 710, in __init__
    errread, errwrite)
  File "/Users/horie/.pyenv/versions/2.7.11/lib/python2.7/subprocess.py", line 1335, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory
==============================================

I'm using python 2.7.11 with pyenv.
Could you give me any advice to fix the error, please?

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

It seems this is trying to invoke pip. You may need to install pip in your pyenv for using some stuff.

Comment 7

[Issei Horie [:is2ei]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #6)
> It seems this is trying to invoke pip. You may need to install pip in your
> pyenv for using some stuff.

Thanks! I installed pip in pyenv and re-created virtualenv directory.
It works now :) I will continue to work on this issue.

Comment 8

[Issei Horie [:is2ei]]

Attachment: bug-1447712-01.diff

I'm trying to implement ToComputedValue and ToCss but I've got a lot of error..
Could you help me to work on this issue? I think I'm stuck.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Reading your patch, I can believe that you got lots of errors.

I encourage you to read the error messages yourself and try to understand what it's saying. If there is anything in the error message that you cannot understand, you can ask here with more specific questions about it. That should be a better approach than just asking for help in general.

Comment 10

[Issei Horie [:is2ei]]

Thanks. Could I ask you one by one?

========================
error[E0599]: no function or associated item named `normal` found for type `values::computed::text::LetterSpacing` in the current scope
     --> /Users/horie/oss/mozilla/mozilla-unified/servo/target/debug/build/style-6aa68b1b19844681/out/properties.rs:16217:69
      |
16217 |         #[inline] pub fn get_initial_value() -> computed_value::T { computed::LetterSpacing::normal() }
      |                                                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ function or associated item not found in `values::computed::text::LetterSpacing`
      | 
     ::: components/style/values/computed/text.rs:29:1
      |
29    | pub struct LetterSpacing(Spacing<Length>);
      | ------------------------------------------ function or associated item `normal` not found for this
========================

This file seems output file and I couldn't find the line in the source code.

> /Users/horie/oss/mozilla/mozilla-unified/servo/target/debug/build/style-6aa68b1b19844681/out/properties.rs

How could I approach to this error?
Sorry for asking beginner question.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

I guess you also need to change word-spacing, which has the same issue.

The way to do it is moving Spacing to be a specified-value-only thing, and make that compute to a Length.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

This is a good question.

out/properties.rs is a generated file, so it could be hard to find where is it from when you are not familiar with the codebase. They are generally from some code in servo/components/style/properties, usually .mako.rs files. One tip for solving this issue is to grep the code for the problematic code, for example you can search "LetterSpacing::normal()" here, and you can find the only place it is mentioned is inherited_text.mako.rs[1], so this is where this expression is from.

To fix this, you can either implement a "normal" function for the computed LetterSpacing, or changing the expression in inherited_text.mako.rs to initial it with zero directly. In this case, I would probably recommend the former because name of the inner struct can be rather long as well.

There is another issue I'd like to raise first since it's related to the issue here. In comment 1, my suggestion is to change the computed LetterSpacing to a wrapper of "Length" (rather than "Spacing<Length>" for the specified value), since the computed value no longer has separate keyword "normal".


[1] https://searchfox.org/mozilla-central/rev/f8e72641772293702e76e75620b777b730b71b72/servo/components/style/properties/longhand/inherited_text.mako.rs#129

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> I guess you also need to change word-spacing, which has the same issue.
> 
> The way to do it is moving Spacing to be a specified-value-only thing, and
> make that compute to a Length.

That's probably true, but let's focus on letter-spacing first in this bug for now...

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #13)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> > I guess you also need to change word-spacing, which has the same issue.
> > 
> > The way to do it is moving Spacing to be a specified-value-only thing, and
> > make that compute to a Length.
> 
> That's probably true, but let's focus on letter-spacing first in this bug
> for now...

I was just saying because they share a type, so it's probably more straight-forward to change both at the same time... But sure, whatever is easier really.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Hmm... that's reasonable.

Issei, you can either continue following the steps in comment 1, or try to fix word-spacing at the same time as well via:
* move the "enum Spacing" from generics::text into specified::text
* keep the LetterSpacing and WordSpacing aliases of Spacing<*>
* impl ToComputedValue for specified LetterSpacing and WordSpacing separately
* change LetterSpacing in values::computed::text to a struct wrapping Length
* change WordSpacing in values::computed::text to a struct wrapping LengthOrPercentage
* impl ToCss for the computed value types to special treat the zero value as "normal"

Comment 16

[Issei Horie [:is2ei]]

Thanks for you advise!
BTW, which IRC channel can I ask a question for this component?

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

We generally use #layout and #servo for this component.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Any news for this bug? Is there anything we can help here?

Comment 19

[Issei Horie [:is2ei]]

Hi Xidorn,

Sorry for delay.. I'm still working on this issue.
If possible, could you give me more hint or docs for this component, please? Maybe I'm not sure what needs to be done...

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

No worries. Just wondering if there is anything blocking you from us.

There probably isn't lots of docs at the moment, sorry.

Generally, we want to keep the original input from CSS text in specified value, but simplify them (via merging equivalent forms of values) in computed value. This bug is a case where we want to merge "normal" value in specified value into zero for computed value, so that we don't need special handling among the layout system for that keyword. So we want to make the computed value types just take length (or length and percentage).

Code-wise, mod values contains majority of types for storing CSS values. CSS values have two forms, specified value (which is value parsed from the CSS source), and computed value (which is what elements get after cascading). And values::specified and values::computed contains types for them correspondingly.

Mod values::generics contains types which have the same shape between specified value and computed value which only differ in types of some components. Like what we do in current code for word-spacing and letter-spacing given both specified and computed values can hold a keyword or a numerical value. We want to change the shape of computed value, so we should move the types from values::generics to values::specified and create new types in values::computed.

We use the ToComputedValue trait for conversion from specified value to computed value. When the two values share the same shape of value, this trait can be derived via #[derive(ToComputedValue)] directly. But in this bug we are changing the shape of the computed value (but not the specified value), so we would need to implement that trait manually for the corresponding types and handle the "normal" keyword in that.

Hope this would be helpful for you!

Comment 21

[Issei Horie [:is2ei]]

Thanks for the info.

I can't build servo.
It seems the module "gfx" is wrong.
https://pastebin.mozilla.org/9086398
Could you help me with this issue?

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

You don't need to worry about Servo. It is no longer a blocker to have Servo build with changes we are going to land in Gecko. We may fix them when syncing changes to Servo manually.

It seems it's an existing issue from variation font implementation, so I think you can just ignore it for now (by not trying build Servo).

Comment 23

[Nour Saffour]

Is this bug up for taking? I saw that the last comment was a year ago

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Yes, that'd be great :)

Comment 25

[Martin McNickle]

I think this has been fixed by in https://bugzilla.mozilla.org/show_bug.cgi?id=1532122 emilio?

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Agreed!