-
Notifications
You must be signed in to change notification settings - Fork 30
Fontsize changes #430
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
Open
silviapfeiffer
wants to merge
1
commit into
w3c:gh-pages
Choose a base branch
from
silviapfeiffer:fontsizeChanges
base: gh-pages
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Fontsize changes #430
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
@silviapfeiffer am I right in thinking that those changes do not then modify the height of a region by changing the value of lines ? If so, I really think this needs to be stated, otherwise people might assume that the region has enough size to fit some number of lines but then after applying CSS to change lineHeight, find that it does not.
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.
We've been there, see #371 (comment) , which is why we added this sentence. The CSS specification can change all those things including the height of the region.
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.
Yes we have! Without wanting to go round in circles, as noted at #371 (comment) adjusting the CSS line height does not affect the value of lines because there seems to be no step that goes back to modify it. If there actually is a step, then it is non-obvious and it would be helpful to point to it in this note, but if there is indeed no such step (and we don't add one), then that also would be worth stating in this note.
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.
linesdoes not get changed, but the height of the region might because thelineHeightdoes. What would you like me to add? I'm very confused!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.
There's no mechanism for the height of the region to change based on
lineHeightchanging though, unless I've missed it? §7.1 Processing Model, bullet 12 step 1, bullet 2, setslineHeightandregionHeightand derives the one time valuelines. Then two bullets later the top is calculated before any CSS settings have been applied. Effectively at this point there is a fixed height.Then in bullet 12 step 2, there's the note introduced here that says that lineHeight might be modified by CSS specification application. How is the region height then recalculated?
Side-note: I couldn't find any spec text that maps the CSS
line-heightvalue to lineHeight.I'm reading the text "Let x be y" in an ES6-ish way, i.e. that the value of x is bound immediately and not re-evaluated whenever it is used.
So, IF my reasoning here is correct, then I'm asking for a sentence to be added to say that even if lineHeight is changed due to CSS, the height of the region is not recalculated. OR, if my reasoning is incorrect (and please do explain), then add something like "which modifies the height of the region via XXX" where XXX is a pointer to a spec section or algorithm.