Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Add a
lang
IDL Attribute to CanvasTextDrawingStyles, and clarify "direction" on same #10873base: main
Are you sure you want to change the base?
Add a
lang
IDL Attribute to CanvasTextDrawingStyles, and clarify "direction" on same #10873Changes from 14 commits
eba506c
caa22ca
488b95b
01e6185
b6df5ee
1b5a1df
152e3a4
60a5971
720ff90
c131fd6
119a4c5
25efd43
369688c
7e5c880
37d7233
e999e9e
f129f9b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have tests that fail if you implement this using "computed style"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To-date there are no tests for preferring the element
dir
attribute over the computed styledirection
property, because we're waiting on data from a stable release to tell us how often the elementdir
attribute differs from the computed style property value. That is, how often would rendering change if we ignored the computed style. The current answer is "not zero but not too many either" but pre-stable data is not reliable. https://chromestatus.com/metrics/feature/timeline/popularity/5235Once we know what the back-compat story is we'll know what needs to be changed in code or spec.
The same goes for the round-tripping of the
canvas
direction = "inherit"` value. We're gathering data in preparation of making the change (though I do have new tests awaiting review that will test the round-trip behavior in WPT, because we all agree on what the behavior should be).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads a bit awkward.
Maybe
You also need to explain what "inherit" means here. You can't refer to the implementation algorithm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've applied you suggestion and made an attempt at clearly explaining what
"inherit"
means.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to use the modern getter/setter language including an internal concept to store the value. As I believe indicated before, you can use
fillStyle
as an example to crib from.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did my best to apply the modern language.
It was not obvious how to integrate it into the existing CanvasTextDrawingStyles spec language because none of the existing attributes use the modern form. In particular, there are three distinct blocks of attribute information: One block defines the getter/setter steps. The next block defines the valid values and what to do with them. Another block defines how they are used in the text preparation algorithm. The first 2 of these blocks should be combined and fully converted to the modern style. I believe that's out of scope for this change but is something I am happy to try to do once this lands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit weird as it's not clear when this happens. I would expect us to compute the inherited language at a given point in time. Or perhaps it's live which means we have to compute it whenever we do a text operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's intended to run whenever the
text preparation algorithm
runs. So it's live in that the process should run whenever something that uses the text preparation is created/drawn. One could of course optimize it but the result should match the live behavior. I've tried making that clear.