Skip to content
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

Should TextFormat.underlineStyle be an enum? #55

Closed
dandclark opened this issue Jul 31, 2023 · 1 comment · Fixed by #64
Closed

Should TextFormat.underlineStyle be an enum? #55

dandclark opened this issue Jul 31, 2023 · 1 comment · Fixed by #64
Labels
needs-spec-edits Issue was resolved but spec needs to be updated

Comments

@dandclark
Copy link
Contributor

In the IDL, TextFormat.underlineStyle and TextFormat.underlineThickness are defined as DOMStrings, and separately the spec text lists the values they can take: https://w3c.github.io/edit-context/#dfn-underlinestyle, https://w3c.github.io/edit-context/#ref-for-dfn-underlinethickness-1.

It might be more straightforward to define these in the IDL as an enum:

enum UnderlineStyle { "none", "solid", "double", "dotted", "dashed", "wavy" };
enum UnderlineThickness { "none", "thin", "thick" };

interface TextFormat {
   ...
    readonly attribute UnderlineStyle underlineStyle;
    readonly attribute UnderlineThickness underlineThickness;
    readonly attribute [DOMString](https://webidl.spec.whatwg.org/#idl-DOMString) underlineColor;
};

Thoughts?
@johanneswilm johanneswilm mentioned this issue Sep 1, 2023
Closed
@dandclark
Copy link
Contributor Author

We resolved to make this change in the TPAC Editing WG meeting:

Dan: Is minor issue. They are currently strings. Enums could make more sense.
Ryosuke: enum seems like more appropriate type.
RESOLUTION: Switch to enums.

@dandclark dandclark added needs-spec-edits Issue was resolved but spec needs to be updated and removed Agenda+ labels Sep 14, 2023
@dandclark dandclark mentioned this issue Sep 15, 2023
Merged
dandclark added a commit that referenced this issue Sep 18, 2023
@dandclark
Make underlineStyle and underlineThickness enums (#64)
9b47961 
Per resolution of #55, change `TextFormat.underlineStyle` and `TextFormat.underlineThickness` to enums.

Also:
- Change https://w3c.github.io/edit-context/#dispatch-text-format-update-event to clarify that the `textFormats` param is an array of [text format](https://w3c.github.io/edit-context/#dfn-text-format)s provided by the [Text Input Service](https://w3c.github.io/edit-context/#dfn-text-input-service), and to use its members as defined in the spec. Previously it was a bit hand-wavy about where these values were coming from.
- In https://w3c.github.io/edit-context/#textformatupdateevent, change the contents of the field descriptions to match those for the other IDL sections in this spec.
- Change the names of the [text format](https://w3c.github.io/edit-context/#dfn-text-format) values to help distinguish between the internal spec terms and the IDL properties.
  
Resolves #55 .
aarongable pushed a commit to chromium/chromium that referenced this issue Sep 19, 2023
@dandclark
[EditContext] Switch TextFormat props to enums
09b3bcb 
Per [1], switch TextFormat.underlineStyle and
TextFormat.underlineThickness to enums.

[1] w3c/edit-context#55

Bug: 999184
Change-Id: I04974a53e6b9399a7d7ca116795544f5e6bada6c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4869357
Reviewed-by: Alex Keng <shihken@microsoft.com>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1198458}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
needs-spec-edits Issue was resolved but spec needs to be updated
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make underlineStyle and underlineThickness enums dandclark/edit-context
1 participant
@dandclark