stylo: all:inherit doesn't work with width for textareas

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: manishearth, Unassigned)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
<!DOCTYPE html>
<style>
textarea {
  all: inherit;
  width: 100%;
}
</style>
<body>
<textarea style="border: 1px solid black;">aaa</textarea>
</body>

will show a screen-width-spanning textarea in Gecko, but in Stylo it will be a default size textarea.

Presumably the way we set default sizes on anonymous inner content doesn't play well with `all:inherit`. 

This happens for textarea and video, but afaict not any other elements
So there are two problems here.

First, we don't respect the position where the "all" property appeared, and put it after it. That's easy to fix, I'll open a separate bug for it, with a test if it doesn't fix any.

With that fixed, then the problem is the deduplication we do. Given how we do the "all" property, and the internal order in the sub-properties, we end up with "inline-width: inherit" after "width: 100%". That one wins and we keep the "inherit".

But that means there's a problem with logical properties in general, as can be demonstrated by:

<!DOCTYPE html>
<style>
div {
  width: 10px;
  inline-size: 100px;
  width: 100%;
  background: red;
  height: 10px;
}
</style>
<div>
</div>

That should show a 100% width div, while in stylo, inline-size wins because we deduplicate the last `width` property declaration with the first `width` property declaration that appeared. Thus, we show a 100px width div.
The second one is pretty annoying. Happy to hear ideas that don't imply destroying our parsing perf :)
Depends on: 1369228
Gah, I guess the first is not relevant after all, and for the first we can just shift the declarations...
Comment hidden (mozreview-request)
I don't think we can use this approach.  CSSOM requires that we re-use the existing declaration: https://drafts.csswg.org/cssom/#set-a-css-declaration (as invoked by setProperty)

Comment 6

a year ago
mozreview-review
Comment on attachment 8873250 [details]
Bug 1369198: Ensure pushing a declaration to a declaration block for parsing always inserts it at the last position.

https://reviewboard.mozilla.org/r/144702/#review148676
Attachment #8873250 - Flags: review?(cam) → review-
Comment hidden (mozreview-request)

Comment 8

a year ago
mozreview-review
Comment on attachment 8873250 [details]
Bug 1369198: Ensure pushing a declaration to a declaration block for parsing always inserts it at the last position.

https://reviewboard.mozilla.org/r/144702/#review149004

Great, thanks! :)
Attachment #8873250 - Flags: review?(cam) → review+

Comment 9

a year ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/86b7357e55e2
Update reftest expectations. r=emilio

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/86b7357e55e2
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.