Closed Bug 1403282 Opened 7 years ago Closed 7 years ago

stylo: attr() does not ignore whitespace

Categories

(Core :: CSS Parsing and Computation, defect, P2)

Product:
Core
Component:
CSS Parsing and Computation
Version:
57 Branch
Platform:
Unspecified
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: mozbz, Assigned: manishearth)

References

(Blocks 1 open bug)

Details

(Keywords: nightly-community)

Attachments

(3 files)

stylo-testcase.html
137 bytes, text/html
Details
Bug 1403282 - stylo: Don't error out on trailing whitespace in attr();
59 bytes, text/x-review-board-request
bzbarsky
: review+
Sylvestre
: approval-mozilla-beta+
Details
Bug 1403282 - stylo: Add reftests for whitespace in attr();
59 bytes, text/x-review-board-request
bzbarsky
: review+
Sylvestre
: approval-mozilla-beta+
Details 
User Agent: Mozilla/5.0 (Windows NT 10.0; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20170923100045

Steps to reproduce:

Example document:

`<!DOCTYPE html><html><body>
<div data-text="World">Hello</div>
<style>
	div::after{ content: attr( data-text ); }
</style></body></html>`


Actual results:

With Stylo enabled (`layout.css.servo.enabled`):
    - the text reads "Hello" and a "Declaration dropped" warning for the `content` value is sent to the Console.
    - with the spaces around the attribute name in `attr()` removed, the text reads "HelloWorld" and no warning is given.

With Stylo disabled, the text reads "HelloWorld" and no warning is given.
In other tested browsers (IE 11, Edge 38, Chrome 61),  the text reads "HelloWorld".


Expected results:

I couldn't see the correct behavior detailed in the brief look I gave the spec, but this is a change from what Firefox used to do, and what IE, Edge and Chrome currently do.
Attached file stylo-testcase.html — 
from comment 0
Thank you!
Confirmed in Nightly 58 x64 20170926100259 de_DE @ Debian Testing (KDE / Radeon RX4 80).
Stylo enabled: Hello
Stylo disabled: HelloWorld
Blocks: stylo-site-issues
Status: UNCONFIRMED → NEW
Has STR: --- → yes
status-firefox57: --- → ?
status-firefox58: --- → affected
Ever confirmed: true
OS: Unspecified → All
Priority: -- → P2
Assignee: nobody → manishearth
[Tracking Requested - why for this release]:
Web-compat issue with stylo.
tracking-firefox57: --- → ?
Comment on attachment 8912407 [details]
Bug 1403282 - stylo: Don't error out on trailing whitespace in attr();

https://reviewboard.mozilla.org/r/183732/#review188936

r=me
Attachment #8912407 - Flags: review?(bzbarsky) → review+
Comment on attachment 8912408 [details]
Bug 1403282 - stylo: Add reftests for whitespace in attr();

https://reviewboard.mozilla.org/r/183734/#review188938

r=me
Attachment #8912408 - Flags: review?(bzbarsky) → review+
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cfcb51808843
stylo: Add reftests for whitespace in attr(); r=bz
Should probably request 57 uplift.
Flags: needinfo?(manishearth)
Comment on attachment 8912407 [details]
Bug 1403282 - stylo: Don't error out on trailing whitespace in attr();

Approval Request Comment
[Feature/Bug causing the regression]: Stylo
[User impact if declined]: Usage of content:attr may not work on some sites
[Is this code covered by automated tests?]: Yes (second patch)
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: The other patch
[Is the change risky?]: No
[Why is the change risky/not risky?]: Minor fix
[String changes made/needed]: No
Flags: needinfo?(manishearth)
Attachment #8912407 - Flags: approval-mozilla-beta?
Attachment #8912408 - Flags: approval-mozilla-beta?
(In reply to Manish Goregaokar [:manishearth] from comment #8)
> https://github.com/servo/servo/pull/18644

This hit autoland as https://hg.mozilla.org/integration/autoland/rev/4263e1c081c9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment on attachment 8912407 [details]
Bug 1403282 - stylo: Don't error out on trailing whitespace in attr();

Seems like a minor change which can save a lot of time and frustrations.
Taking it
Should be in 57b4
Attachment #8912407 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #2)
> Confirmed in Nightly 58 x64 20170926100259 de_DE @ Debian Testing (KDE / Radeon RX480).
> Stylo enabled: Hello
> Stylo disabled: HelloWorld

Verified fixed in Nightly 58 x84 20170928100123 de_DE @ Debian Testing (KDE / Radeon RX480).
attachment 8912390 [details] with Stylo enabled & disabled: HelloWorld
Attachment #8912408 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Manish Goregaokar [:manishearth] from comment #11)
> [Is this code covered by automated tests?]: Yes (second patch)
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Manish's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #16)
> (In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #2)
> > Confirmed in Nightly 58 x64 20170926100259 de_DE @ Debian Testing (KDE / Radeon RX480).
> > Stylo enabled: Hello
> > Stylo disabled: HelloWorld
> 
> Verified fixed in Nightly 58 x64 20170928100123 de_DE @ Debian Testing (KDE / Radeon RX480).
> attachment 8912390 [details] with Stylo enabled & disabled: HelloWorld

Verified fixed in Beta 57.0b4 x64 20170928180207 de_DE @ Debian Testing.
attachment 8912390 [details] with Stylo enabled & disabled: HelloWorld
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: