Bug 91091 – line-height decreases when zooming in

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: New Bugs

[Jonathan Alley]

Created attachment 151945 [details]
Simple test page that demonstrates the issue

On the body element line-height is set to 1.4. On a div I have the font-size set to 12px and so for the div the inspector shows a computed line-height of 16px. If you zoom in twice, the line-height is shown to be 11px. This isn't an inspector bug as if, in Chrome anyways, I use getComputedStyle on the div after the zoom the line-height is still reduced.

[Emil A Eklund]

Created attachment 190153 [details]
Patch

[Build Bot]

Comment on attachment 190153 [details]
Patch

Attachment 190153 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16743661

New failing tests:
fast/css/font-shorthand-from-longhands.html
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
css3/calc/line-height.html
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/ruby/ruby-line-height.html
fast/css/getComputedStyle/line-height.html
fast/css/getComputedStyle/computed-style-font.html
fast/forms/textAreaLineHeight.html

[WebKit Review Bot]

Attachment 190153 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/calc/font-size-fractional-expected.txt', u'LayoutTests/css3/calc/line-height-expected.txt', u'LayoutTests/editing/pasteboard/data-transfer-items-expected.txt', u'LayoutTests/editing/pasteboard/onpaste-text-html-expected.txt', u'LayoutTests/fast/css/font-shorthand-from-longhands-expected.txt', u'LayoutTests/fast/css/font-shorthand-from-longhands.html', u'LayoutTests/fast/css/getComputedStyle/computed-style-expected.txt', u'LayoutTests/fast/css/getComputedStyle/computed-style-font-expected.txt', u'LayoutTests/fast/css/getComputedStyle/computed-style-without-renderer-expected.txt', u'LayoutTests/fast/css/getComputedStyle/line-height-expected.txt', u'LayoutTests/fast/css/getComputedStyle/line-height.html', u'LayoutTests/fast/css/getComputedStyle/script-tests/computed-style-font.js', u'LayoutTests/fast/events/ondrop-text-html-expected.txt', u'LayoutTests/fast/frames/seamless/seamless-inherited-document-style-expected.txt', u'LayoutTests/fast/frames/seamless/seamless-inherited-document-style.html', u'LayoutTests/fast/ruby/ruby-line-height-expected.txt', u'LayoutTests/fast/ruby/script-tests/ruby-line-height.js', u'LayoutTests/platform/chromium-linux/fast/forms/textAreaLineHeight-expected.png', u'LayoutTests/platform/chromium-win/fast/forms/textAreaLineHeight-expected.txt', u'LayoutTests/svg/css/getComputedStyle-basic-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/StyleBuilder.cpp']" exit_code: 1
LayoutTests/platform/chromium-linux/fast/forms/textAreaLineHeight-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 190153 [details]
Patch

Attachment 190153 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16752785

New failing tests:
editing/pasteboard/paste-match-style-001.html
editing/inserting/insert-div-022.html

[Build Bot]

Comment on attachment 190153 [details]
Patch

Attachment 190153 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16663187

New failing tests:
fast/css/font-shorthand-from-longhands.html
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
css3/calc/line-height.html
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/ruby/ruby-line-height.html
fast/css/getComputedStyle/line-height.html
fast/css/getComputedStyle/computed-style-font.html
fast/forms/textAreaLineHeight.html

[Emil A Eklund]

Created attachment 191827 [details]
Patch

[Levi Weintraub]

Comment on attachment 191827 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191827&action=review

Otherwise this looks good.

> LayoutTests/platform/chromium-win/editing/inserting/insert-div-022-expected.txt:10
> +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 1 of #text > B > SPAN > P > BODY > HTML > #document to 1 of #text > B > SPAN > P > BODY > HTML > #document toDOMRange:range from 1 of #text > B > SPAN > P > BODY > HTML > #document to 1 of #text > B > SPAN > P > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE

I don't understand this change.

> LayoutTests/platform/chromium-win/editing/inserting/insert-div-022-expected.txt:45
> +caret: position 1 of child 0 {#text} of child 0 {B} of child 0 {SPAN} of child 6 {P} of body

Likewise... How did this patch cause this diff?

[Emil A Eklund]

(In reply to comment #7)
> > LayoutTests/platform/chromium-win/editing/inserting/insert-div-022-expected.txt:45
> > +caret: position 1 of child 0 {#text} of child 0 {B} of child 0 {SPAN} of child 6 {P} of body
> 
> Likewise... How did this patch cause this diff?

The editing code tries to ensure that the line-height is preserved when splitting and is relying on the computedStyle.lineHeight to return the specified/cascaded value (as opposed to the computed value). Trying to figure out a non-hacky way to deal with this.

[Emil A Eklund]

Created attachment 192088 [details]
Patch

[WebKit Review Bot]

Comment on attachment 192088 [details]
Patch

Attachment 192088 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17087113

New failing tests:
fast/events/ondrop-text-html.html

[Emil A Eklund]

*** Bug 103538 has been marked as a duplicate of this bug. ***

[Emil A Eklund]

Created attachment 192303 [details]
Patch

[Levi Weintraub]

Comment on attachment 192303 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192303&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2965
> +        // This is imperfect, because it doesn't include the zoom factor and the real computation
> +        // for how high to be in pixels does include things like minimum font size and the zoom factor.

This sentence isn't quite right. "but does include"?

[Emil A Eklund]

(In reply to comment #13)
> (From update of attachment 192303 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192303&action=review
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2965
> > +        // This is imperfect, because it doesn't include the zoom factor and the real computation
> > +        // for how high to be in pixels does include things like minimum font size and the zoom factor.
> 
> This sentence isn't quite right. "but does include"?

I didn't write that but I'll make sure to fix it :)

[Emil A Eklund]

Created attachment 192310 [details]
Patch

[Levi Weintraub]

Comment on attachment 192310 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192310&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2978
> +            // Line-height depends on font thus we don't want to copy the computed value but rather the specified/cascaded value.
> +            value = getCascadedLineHeight(styledNode(), m_pseudoElementSpecifier, m_node->document()->renderView());

Cascaded does sound a little weird. The idea is that this returns a value if it's set been set, and otherwise nothing so we don't end up with misleading line-height styles when they haven't actually been set, no?

[Build Bot]

Comment on attachment 192310 [details]
Patch

Attachment 192310 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17011328

New failing tests:
fast/css/font-shorthand-from-longhands.html
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
media/track/track-cue-rendering-on-resize.html
css3/calc/line-height.html
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/ruby/ruby-line-height.html
fast/css/getComputedStyle/line-height.html
fast/css/getComputedStyle/computed-style-font.html

[Build Bot]

Comment on attachment 192310 [details]
Patch

Attachment 192310 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17000673

New failing tests:
fast/css/font-shorthand-from-longhands.html
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
media/track/track-cue-rendering-on-resize.html
css3/calc/line-height.html
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/ruby/ruby-line-height.html
fast/css/getComputedStyle/line-height.html
fast/css/getComputedStyle/computed-style-font.html

[Emil A Eklund]

Will rebasline mac, efl, qt and gtk post-commit.

[Brent Fulgham]

This no longer seems to be an issue. STP 236 shows the same behavior and values as Chrome and Firefox after zooming in twice.