Implement the line-height calculation quirk #11853

nox · 2016-06-24T13:17:01Z

This change is

highfive · 2016-06-24T13:17:05Z

Heads up! This PR modifies the following files:

@bholley: components/style/selector_matching.rs

highfive · 2016-06-24T13:17:06Z

Warning

These commits modify style and layout code, but no tests are modified. Please consider adding a test!

nox · 2016-06-24T13:17:07Z

@bors-servo try

bors-servo · 2016-06-24T13:17:10Z

⌛ Trying commit f1d1c1a with merge 660524e...


        Auto merge of #11853 - nox:quirk-cobain, r=<try>

Implement the line-height calculation quirk

jdm · 2016-06-24T13:34:22Z

@notriddle Would you be interested in reviewing this change?

bors-servo · 2016-06-24T14:09:32Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

notriddle · 2016-06-24T14:27:58Z

@jdm Sure.

KiChjang · 2016-06-24T14:28:36Z

@bors-servo delegate=notriddle

bors-servo · 2016-06-24T14:28:37Z

✌️ @notriddle can now approve this pull request

notriddle · 2016-06-24T14:43:06Z

components/layout/inline.rs

+    border.border_left_width == Au(0) &&
+    border.border_right_width == Au(0) &&
+    padding.padding_left.is_definitely_zero() &&
+    padding.padding_right.is_definitely_zero()


is_definitely_zero is, as the name implies, not an exhaustive test for whether the "used property" of a value is zero. The spec asks for the used value.

How hard would it be to get the used value here?

Shouldn't be hard; use the self.border_padding field.

Edit: Never mind, this is not called at a point where that is available.

notriddle · 2016-06-24T14:44:42Z

Other than that and not having any tests, LGMT.

notriddle · 2016-06-24T14:59:08Z

Looking through the IRC backlog, it looks like the used value only exists after layout. So no code changes needed, just adding a reftest should work.

Ms2ger · 2016-06-24T15:06:37Z

The definitely bits scare me. Can we produce a test where the used value is zero, but not definitely zero?

nox · 2016-06-24T15:08:32Z

It isn't scary, it just means it won't catch cases where the padding is calc(10% - 10px) and the containing block is 100px wide. I'm not even sure other UAs support that, it goes against the spirit of the spec (IMO), and Servo is definitely unable to get the used value at that point in layout, we would need to call compute_minimum_ascent_and_descent in a later layout pass and for now @mbrubeck told me to just ignore that case.

Ms2ger · 2016-06-24T15:11:19Z

If we willfully ignore the spec, we should file an issue.

notriddle · 2016-06-24T15:41:41Z

If none of the browsers implement the spec as written, we should file a bug against the spec.

notriddle · 2016-06-24T15:48:19Z

components/layout/inline.rs, line 1833 [r1] (raw file):

Previously, notriddle (Michael Howell) wrote…

is_definitely_zero is, as the name implies, not an exhaustive test for whether the "used property" of a value is zero. The spec asks for the used value.

How hard would it be to get the used value here?

OK, guess getting a truly used value would be too much of a hassle, and probably not web compatible anyway.

Comments from Reviewable

notriddle · 2016-06-24T15:51:30Z

Reviewed 1 of 3 files at r1.
Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

notriddle · 2016-06-24T15:53:04Z

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

emilio · 2016-07-05T06:33:16Z

I don't think we're deviating from the spec. The definition of used value is:

The used value is the result of taking the computed value and completing any remaining
calculations to make it the absolute theoretical value used in the layout of the document. If the
property does not apply to this element, then the element has no used value for that property.

So I don't think we need the specified value at all here, the computed value is more than enough.

https://drafts.csswg.org/css-cascade-4/#used-value

nox · 2016-07-05T07:24:25Z

@emilio The problem is that the used value is "more computed" than the computed values, and that calc() values maybe shouldn't be subject to the quirk.

emilio · 2016-07-05T07:38:14Z

Ahh, I get it, sorry then... Yeah, this wouldn't catch less-tricky cases as "computed value is 10%", but width is 0 thing, right?

In that case I agree with @Ms2ger, a spec bug should be open at least, though given how servo does layout I doubt it's possible to fix this easily.

zcorpan · 2016-07-06T04:29:38Z

This quirk should also be present in limited-quirks mode.

pcwalton · 2016-07-25T23:03:03Z

components/layout/inline.rs

+                SpecificFragmentInfo::ScannedText(_) => true,
+                _ => false,
+            }
+        })


nit: How about self.fragments.fragments.iter().any(Fragment::is_scanned_text_fragment)?


        Implement the line-height calculation quirk

jdm · 2016-09-17T01:37:54Z

It's not clear to me from reading the previous comments on what the status is here. Are we waiting on some other piece of work, or spec discussion? I seem to recall @nox mentioning that these changes were incomplete in some way, but I might be misremembering. Who's responsible for taking the next step here?

notriddle · 2016-09-17T02:09:02Z

"The fix is completely wrong." Either he fixes it or it goes back into the big pot of unassigned issues.

Ms2ger · 2016-09-21T14:29:14Z

<nox> Ms2ger: Go ahead and close it.

highfive assigned pcwalton Jun 24, 2016

highfive added the S-awaiting-review label Jun 24, 2016

bors-servo added a commit that referenced this pull request Jun 24, 2016

Auto merge of #11853 - nox:quirk-cobain, r=<try>

660524e

Implement the line-height calculation quirk

notriddle added the C-needs-test label Jun 24, 2016

notriddle reviewed Jun 24, 2016
View changes

notriddle added S-needs-tests and removed C-needs-test S-awaiting-review labels Jun 24, 2016

nox force-pushed the nox:quirk-cobain branch from f1d1c1a to d969678 Jun 24, 2016

highfive added the S-awaiting-review label Jun 24, 2016

notriddle removed the S-awaiting-review label Jun 24, 2016

notriddle unassigned pcwalton Jul 5, 2016

notriddle self-assigned this Jul 5, 2016

jdm mentioned this pull request Jul 5, 2016

Implement line-height quirks #11704

Open

zcorpan mentioned this pull request Jul 6, 2016

Use computed value instead of used value whatwg/quirks#8

Open

pcwalton reviewed Jul 25, 2016
View changes

components/layout/inline.rs Outdated

SpecificFragmentInfo::ScannedText(_) => true,

_ => false,

}

})

This comment has been minimized.

Sign in to view

Copy link

pcwalton Jul 25, 2016
Contributor

nit: How about self.fragments.fragments.iter().any(Fragment::is_scanned_text_fragment)?

Implement the line-height calculation quirk

Loading status checks…

ff4ceb0

nox force-pushed the nox:quirk-cobain branch from d969678 to ff4ceb0 Jul 26, 2016

highfive added the S-awaiting-review label Jul 26, 2016

jdm added S-awaiting-answer and removed S-awaiting-review labels Sep 17, 2016

Ms2ger closed this Sep 21, 2016

servo / servo

Implement the line-height calculation quirk #11853

Implement the line-height calculation quirk #11853

nox commented Jun 24, 2016 •

edited by larsbergstrom

highfive commented Jun 24, 2016

highfive commented Jun 24, 2016

nox commented Jun 24, 2016

bors-servo commented Jun 24, 2016

jdm commented Jun 24, 2016 •

edited

bors-servo commented Jun 24, 2016

notriddle commented Jun 24, 2016

KiChjang commented Jun 24, 2016

bors-servo commented Jun 24, 2016

This comment has been minimized.

This comment has been minimized.

notriddle commented Jun 24, 2016

notriddle commented Jun 24, 2016

Ms2ger commented Jun 24, 2016

nox commented Jun 24, 2016

Ms2ger commented Jun 24, 2016

notriddle commented Jun 24, 2016

notriddle commented Jun 24, 2016

notriddle commented Jun 24, 2016

notriddle commented Jun 24, 2016

emilio commented Jul 5, 2016 •

edited

nox commented Jul 5, 2016

emilio commented Jul 5, 2016 •

edited

zcorpan commented Jul 6, 2016

This comment has been minimized.

jdm commented Sep 17, 2016

notriddle commented Sep 17, 2016

Ms2ger commented Sep 21, 2016

servo / servo

Join GitHub today

Implement the line-height calculation quirk #11853

Implement the line-height calculation quirk #11853

Conversation

nox commented Jun 24, 2016 • edited by larsbergstrom

highfive commented Jun 24, 2016

highfive commented Jun 24, 2016

nox commented Jun 24, 2016

bors-servo commented Jun 24, 2016

jdm commented Jun 24, 2016 • edited

bors-servo commented Jun 24, 2016

notriddle commented Jun 24, 2016

KiChjang commented Jun 24, 2016

bors-servo commented Jun 24, 2016

This comment has been minimized.

notriddle Jun 24, 2016

This comment has been minimized.

pcwalton Jul 25, 2016 • edited

notriddle commented Jun 24, 2016

notriddle commented Jun 24, 2016

Ms2ger commented Jun 24, 2016

nox commented Jun 24, 2016

Ms2ger commented Jun 24, 2016

notriddle commented Jun 24, 2016

notriddle commented Jun 24, 2016

notriddle commented Jun 24, 2016

notriddle commented Jun 24, 2016

emilio commented Jul 5, 2016 • edited

nox commented Jul 5, 2016

emilio commented Jul 5, 2016 • edited

zcorpan commented Jul 6, 2016

This comment has been minimized.

pcwalton Jul 25, 2016

jdm commented Sep 17, 2016

notriddle commented Sep 17, 2016

Ms2ger commented Sep 21, 2016

nox commented Jun 24, 2016 •

edited by larsbergstrom

jdm commented Jun 24, 2016 •

edited

pcwalton Jul 25, 2016 •

edited

emilio commented Jul 5, 2016 •

edited

emilio commented Jul 5, 2016 •

edited