Skip to content

[cssom][all] Add CSSOMString and replace DOMString usage with it #1266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Apr 21, 2017
Merged

[cssom][all] Add CSSOMString and replace DOMString usage with it #1266

merged 5 commits into from
Apr 21, 2017

Conversation

@SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented Apr 21, 2017 •
edited
Loading

CSSOMString is an IDL typedef of either USVString or DOMString, chosen by implementations.

CSSWG resolution: #1217 (comment). Fixes #1217.

Each replaced occurrence is one of:

  • CSS syntax
  • A name (for example a property name) that also occurs in CSS syntax
  • Stylesheet::type, which is always text/css.
  • Stylesheet::title, which is set from the eponymous HTML content attribute of <style> and <link> elements.

@SimonSapin SimonSapin added the cssom-1 Current Work label Apr 21, 2017
zcorpan
zcorpan reviewed Apr 21, 2017
View reviewed changes
cssom/Overview.bs

Or, alternatively:

<pre class="def lang-webidl">
Copy link
Contributor

@zcorpan zcorpan Apr 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use class="idl extract" here instead.

Copy link
Contributor Author

@SimonSapin SimonSapin Apr 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With that Bikeshed gives:

FATAL ERROR: Multiple local 'typedef' <dfn>s have the same linking text 'CSSOMString'.

Copy link
Contributor

@zcorpan zcorpan Apr 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... @tabatkins should this work?

Copy link
Member

@tabatkins tabatkins Apr 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't added the "extract" support yet. (The bug is still open!)

(And in any case, I think it's probably better to do exactly this - instead of claiming something is WebIDL and then opting out, just ask it to be highlighted as WebIDL but otherwise be meaningless text.)

zcorpan
zcorpan reviewed Apr 21, 2017
View reviewed changes
cssom/Overview.bs Outdated
This choice effectively allows implementations to do this replacement,
but does not require it.

This enables an implementation to use UTF-8 internally to represent strings in memory.
Copy link
Contributor

@zcorpan zcorpan Apr 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/This/Using {{USVString}}/

Copy link
Contributor Author

@SimonSapin SimonSapin Apr 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

zcorpan
zcorpan reviewed Apr 21, 2017
View reviewed changes
cssom/Overview.bs Outdated

On the other hand,
implementations that internally represent strings as 16-bit <a>code units</a>
may prefer to avoid the cost of doing this replacement.
Copy link
Contributor

@zcorpan zcorpan Apr 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use may in a note

Copy link
Contributor Author

@SimonSapin SimonSapin Apr 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced with “might”.

@zcorpan
Copy link
Contributor

zcorpan commented Apr 21, 2017

  • Stylesheet::title, which is set from the eponymous HTML content attribute of <style> and <link> elements.

Can you explain why this one shouldn't be DOMString?

@zcorpan
Copy link
Contributor

zcorpan commented Apr 21, 2017

(Thanks for fixing link errors in cssom!)

@SimonSapin
Copy link
Contributor Author

SimonSapin commented Apr 21, 2017

  • Stylesheet::title, which is set from the eponymous HTML content attribute of <style> and <link> elements.

Can you explain why this one shouldn't be DOMString?

I did hesitate about this one, it just seemed more “within style system territory”. I don’t feel strongly about it, though. In Stylo we can (and possibly already do) keep this on the C++ side rather than the Rust side of the bindings. (And Servo needs to decide what to do about every DOMString, not just stylesheet titles.)

@zcorpan
Copy link
Contributor

zcorpan commented Apr 21, 2017

So title on the HTML side is on HTMLElement and is DOMString. Then "create a CSS stylesheet" the title content attribute is passed for the title, which is then updated if the content attribute is changed. styleSheet.title then returns that value... which is a DOMString.

I'm open to changing how this works, but since the string will exist as a DOMString anyway, it seems kinda pointless to use CSSOMString on StyleSheet.

@SimonSapin
Copy link
Contributor Author

SimonSapin commented Apr 21, 2017

Changed Stylesheet::title back to DOMString?.

@SimonSapin
[all] Replace most occurrences DOMString with CSSOMString.
b937a92 
CSSWG resolution:
w3c#1217 (comment)

Fix w3c#1217.

Each occurrence is one of:

* CSS syntax
* A name (for example a property name) that also occurs in CSS syntax
* `Stylesheet::type`, which is always `text/css`.

Not replaced:

* `Stylesheet::title`, which is set from the eponymous HTML content attribute
  of [`<style>`](https://html.spec.whatwg.org/multipage/semantics.html#attr-style-title)
  and [`<link>`](https://html.spec.whatwg.org/multipage/semantics.html#attr-link-title)
  elements.
  These contant attributes are reflected as `HTMLElement::title` DOM attributes,
  where they are `DOMString`.
@zcorpan zcorpan mentioned this pull request Apr 21, 2017
Open
@zcorpan zcorpan merged commit 4049ec2 into w3c:master Apr 21, 2017
@zcorpan zcorpan mentioned this pull request Apr 21, 2017
Open
@zcorpan
Copy link
Contributor

zcorpan commented Apr 21, 2017

Filed web-platform-tests/wpt#5641 to follow up on tests. The href change affects everyone, so might also need bugs on browsers.

@SimonSapin SimonSapin mentioned this pull request Apr 21, 2017
Closed
@SimonSapin SimonSapin deleted the cssomstring branch May 4, 2017 17:18
@SimonSapin SimonSapin mentioned this pull request May 11, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tabatkins tabatkins tabatkins left review comments

@zcorpan zcorpan zcorpan approved these changes

Assignees

No one assigned

Labels

cssom-1 Current Work

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@SimonSapin @zcorpan @tabatkins