Issue 82083002: Move viewport unit resolution to style recalc time

Issue 82083002: Move viewport unit resolution to style recalc time (Closed)

Created:
7 years, 1 month ago by Timothy Loh

Modified:
6 years, 11 months ago

Reviewers:
johnme, dglazkov, rune, esprehn, kenneth.r.christiansen, eseidel, pdr., Julien - ping for review

CC:
blink-reviews, shans, eae+blinkwatch, apavlov+blink_chromium.org, adamk+blink_chromium.org, pdr, Steve Block, rwlbuis, dino_apple.com, alancutter (OOO until 2018), bemjb+rendering_chromium.org, dstockwell, dglazkov+blink, jchaffraix+rendering, Eric Willigers, kenneth.christiansen, rjwright, zoltan1, krit, darktears, leviw+renderwatch, Mike Lawther (Google), f(malita), Stephen Chennney, Xianzhu

Base URL:
https://chromium.googlesource.com/chromium/blink@master

Visibility:
Public.

More Reviews

Description

Move viewport unit resolution to style recalc time This patch moves the resolution of viewport units to style recalc time. Currently viewport units are left unresolved during style recalcs which leads to several problems (see linked bugs). Moving the resolution will fix these problems, as well as reduce the plumbing that goes on. This patch is touches a lot of files since the valueForLength functions now no longer need a RenderView, but the interesting changes are in: - CSSToLengthConversionData -> CSSPrimitiveValue, for moving resolution to style recalc time - Length / LengthFunction, no longer needs to know about viewport units - FrameView -> Document -> MatchedPropertiesCache, for scheduling style recalcs upon resize Viewport Lengths are currently also used in viewport descriptions (that is, @viewport and the viewport meta tag). I've added to Length two more enum values (DeviceWidth and DeviceHeight), alongside ExtendToZoom. Note that getComputedStyle will now do the right thing and return pixel values when viewport units are used. This behaviour matches Firefox and the css3-cascade spec. I've added a test for the most starred bug (124331) and for the various other issues fixed by this patch I'll make test-only patches. Note that calc integration with viewport units will be fixed separately to simplify reviews. TBR=dglazkov@chromium.org BUG=124331, 137617, 125709, 261298, 173407, 322365, 310874, 311465 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164448

Patch Set 1 #

Patch Set 2 : #

Total comments: 1

Patch Set 3 : rebased.. #

Total comments: 25

Patch Set 4 : clean up silly things in added test #

Patch Set 5 : address a few comments #

Patch Set 6 : rebased #

Patch Set 7 : rebased! #

Patch Set 8 : use length enums instead of magic constants #

Patch Set 9 : re-resolve viewport descriptors on resize #

Patch Set 10 : handle pseudoelements and shadow dom #

Patch Set 11 : rebased #

Total comments: 9

Patch Set 12 : rebased :< #

Patch Set 13 : fix zoom handling #

Patch Set 14 : actually fix zoom handling #

Total comments: 7

Patch Set 15 : move bit added for better packing #

Patch Set 16 : rebased #

Patch Set 17 : add test for browser zoom #

Total comments: 2

Patch Set 18 : fix compile on mac #

Total comments: 10

Patch Set 19 : rebased #

Patch Set 20 : addressed comments #

Patch Set 21 : rebased :\ #

Total comments: 8

Patch Set 22 : rebased and make test pass #

Patch Set 23 : rename browser zoom to page zoom #

Created: 6 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+875 lines, -778 lines)			Patch
M	LayoutTests/animations/interpolation/line-height-interpolation-expected.txt	View	1	1 chunk	+12 lines, -12 lines	0 comments	Download
M	LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html	View		1 chunk	+116 lines, -101 lines	0 comments	Download
M	LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle-expected.txt	View		4 chunks	+36 lines, -36 lines	0 comments	Download
A	LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-page-zoom.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22	1 chunk	+34 lines, -0 lines	0 comments	Download
A	LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-page-zoom-expected.txt	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22	1 chunk	+16 lines, -0 lines	0 comments	Download
A	LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-resize.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21	1 chunk	+46 lines, -0 lines	0 comments	Download
A	LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-resize-expected.txt	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21	1 chunk	+35 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/css/computed-offset-with-zoom.html	View	13	1 chunk	+2 lines, -2 lines	0 comments	Download
M	LayoutTests/fast/css/computed-offset-with-zoom-expected.txt	View		2 chunks	+8 lines, -8 lines	0 comments	Download
M	LayoutTests/fast/masking/parsing-clip-path-shape.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	1 chunk	+6 lines, -6 lines	0 comments	Download
M	LayoutTests/fast/masking/parsing-clip-path-shape-expected.txt	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	1 chunk	+6 lines, -6 lines	0 comments	Download
M	LayoutTests/fast/shapes/parsing/parsing-shape-lengths.html	View	1	1 chunk	+3 lines, -3 lines	0 comments	Download
M	LayoutTests/fast/shapes/parsing/parsing-shape-lengths-expected.txt	View	1	1 chunk	+3 lines, -3 lines	0 comments	Download
M	LayoutTests/platform/linux/virtual/legacy-animations-engine/animations/interpolation/line-height-interpolation-expected.txt	View	1	1 chunk	+12 lines, -12 lines	0 comments	Download
M	LayoutTests/platform/win/virtual/legacy-animations-engine/animations/interpolation/line-height-interpolation-expected.txt	View	1	1 chunk	+12 lines, -12 lines	0 comments	Download
M	LayoutTests/virtual/legacy-animations-engine/animations/interpolation/line-height-interpolation-expected.txt	View	1	1 chunk	+12 lines, -12 lines	0 comments	Download
M	Source/core/animation/AnimatableLengthTest.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/animation/css/CSSAnimatableValueFactory.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	2 chunks	+2 lines, -8 lines	0 comments	Download
M	Source/core/css/CSSCalculationValue.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	3 chunks	+6 lines, -4 lines	0 comments	Download
M	Source/core/css/CSSCalculationValueTest.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/css/CSSComputedStyleDeclaration.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21	18 chunks	+27 lines, -34 lines	0 comments	Download
M	Source/core/css/CSSGradientValue.cpp	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/css/CSSLengthFunctions.h	View		1 chunk	+5 lines, -5 lines	0 comments	Download
M	Source/core/css/CSSLengthFunctions.cpp	View	1 2 3 4 5 6 7	5 chunks	+11 lines, -59 lines	0 comments	Download
M	Source/core/css/CSSMatrix.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/css/CSSPrimitiveValue.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21	3 chunks	+1 line, -4 lines	0 comments	Download
M	Source/core/css/CSSPrimitiveValue.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21	7 chunks	+16 lines, -50 lines	0 comments	Download
M	Source/core/css/CSSPrimitiveValueMappings.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	1 chunk	+0 lines, -2 lines	0 comments	Download
M	Source/core/css/CSSToLengthConversionData.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	1 chunk	+14 lines, -17 lines	0 comments	Download
M	Source/core/css/CSSToLengthConversionData.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	2 chunks	+56 lines, -0 lines	0 comments	Download
M	Source/core/css/MediaQueryEvaluator.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	1 chunk	+2 lines, -1 line	0 comments	Download
M	Source/core/css/resolver/FontBuilder.h	View		2 chunks	+3 lines, -0 lines	0 comments	Download
M	Source/core/css/resolver/FontBuilder.cpp	View	1 2 3 4 5 6 7 8 9 10	2 chunks	+16 lines, -9 lines	0 comments	Download
M	Source/core/css/resolver/MatchedPropertiesCache.h	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/css/resolver/MatchedPropertiesCache.cpp	View	1	1 chunk	+12 lines, -0 lines	0 comments	Download
M	Source/core/css/resolver/StyleBuilderConverter.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21	1 chunk	+0 lines, -2 lines	0 comments	Download
M	Source/core/css/resolver/StyleBuilderCustom.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21	1 chunk	+0 lines, -2 lines	0 comments	Download
M	Source/core/css/resolver/StyleResolver.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	2 chunks	+3 lines, -1 line	0 comments	Download
M	Source/core/css/resolver/StyleResolver.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21	5 chunks	+15 lines, -1 line	0 comments	Download
M	Source/core/css/resolver/StyleResolverState.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/css/resolver/ViewportStyleResolver.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	2 chunks	+18 lines, -16 lines	0 comments	Download
M	Source/core/dom/Document.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21	2 chunks	+6 lines, -0 lines	0 comments	Download
M	Source/core/dom/Document.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21	5 chunks	+15 lines, -7 lines	0 comments	Download
M	Source/core/dom/TreeScope.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/dom/TreeScope.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21	2 chunks	+12 lines, -0 lines	0 comments	Download
M	Source/core/dom/ViewportDescription.cpp	View	1 2 3 4 5 6 7	1 chunk	+6 lines, -6 lines	0 comments	Download
M	Source/core/frame/FrameView.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	1 chunk	+4 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLAreaElement.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21	1 chunk	+8 lines, -9 lines	0 comments	Download
M	Source/core/html/HTMLMetaElement-in.cpp	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/rendering/RenderBR.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/rendering/RenderBlock.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21	4 chunks	+5 lines, -8 lines	0 comments	Download
M	Source/core/rendering/RenderBox.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	37 chunks	+98 lines, -131 lines	0 comments	Download
M	Source/core/rendering/RenderBoxModelObject.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	15 chunks	+36 lines, -43 lines	0 comments	Download
M	Source/core/rendering/RenderFlexibleBox.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/RenderFlexibleBox.cpp	View		2 chunks	+6 lines, -7 lines	0 comments	Download
M	Source/core/rendering/RenderGrid.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/RenderInline.cpp	View	1 2 3 4 5 6	2 chunks	+2 lines, -4 lines	0 comments	Download
M	Source/core/rendering/RenderMenuList.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/RenderObject.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	3 chunks	+4 lines, -4 lines	0 comments	Download
M	Source/core/rendering/RenderReplaced.cpp	View	1 2 3 4 5 6	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/rendering/RenderScrollbarPart.cpp	View		2 chunks	+12 lines, -14 lines	0 comments	Download
M	Source/core/rendering/RenderTable.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	6 chunks	+7 lines, -10 lines	0 comments	Download
M	Source/core/rendering/RenderTableCell.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/RenderTableSection.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	2 chunks	+2 lines, -3 lines	0 comments	Download
M	Source/core/rendering/RenderText.h	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/rendering/RenderThemeChromiumMac.mm	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/RenderView.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	1 chunk	+2 lines, -4 lines	0 comments	Download
M	Source/core/rendering/RenderView.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	2 chunks	+7 lines, -20 lines	0 comments	Download
M	Source/core/rendering/RootInlineBox.cpp	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/style/RenderStyle.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	6 chunks	+14 lines, -4 lines	0 comments	Download
M	Source/core/rendering/style/RenderStyle.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	8 chunks	+18 lines, -19 lines	0 comments	Download
M	Source/core/rendering/svg/RenderSVGRoot.cpp	View	1 2 3 4 5 6 7 8 9 10	5 chunks	+7 lines, -6 lines	0 comments	Download
M	Source/core/svg/SVGSVGElement.cpp	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/platform/Length.h	View	1 2 3 4 5 6 7	3 chunks	+4 lines, -13 lines	0 comments	Download
M	Source/platform/LengthFunctions.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	2 chunks	+2 lines, -7 lines	0 comments	Download
M	Source/web/PageScaleConstraintsSet.cpp	View	1 2 3 4 5 6 7	4 chunks	+5 lines, -5 lines	0 comments	Download
M	Source/web/WebViewImpl.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21	2 chunks	+3 lines, -3 lines	0 comments	Download

Messages

Total messages: 39 (0 generated)

Expand Messages | Collapse Messages

esprehn

Awesome patch, great to see all the passing of the RenderView go away. The -1 ...

7 years ago (2013-12-04 06:02:25 UTC) #2

Timothy Loh

I'm not sure what to do yet with the -1%/-2% in viewport descriptions. Possibly a ...

7 years ago (2013-12-04 07:17:39 UTC) #3

kenneth.r.christiansen

It is not just viewport meta, but also the @viewport rule which allows vh, vw ...

7 years ago (2013-12-04 07:25:42 UTC) #4

rune

On 2013/12/04 07:17:39, Timothy Loh wrote: > I'm not sure what to do yet with ...

7 years ago (2013-12-04 07:43:11 UTC) #5

rune

https://codereview.chromium.org/82083002/diff/70001/Source/core/css/resolver/ViewportStyleResolver.cpp File Source/core/css/resolver/ViewportStyleResolver.cpp (right): https://codereview.chromium.org/82083002/diff/70001/Source/core/css/resolver/ViewportStyleResolver.cpp#newcode187 Source/core/css/resolver/ViewportStyleResolver.cpp:187: return primitiveValue->computeLength<Length>(CSSToLengthConversionData(m_document->renderStyle(), m_document->renderStyle(), m_document->renderView(), 1.0f)); This change means also ...

7 years ago (2013-12-04 07:44:05 UTC) #6

rune

On 2013/12/04 07:44:05, rune - CET wrote: > This change means also viewport descriptors need ...

7 years ago (2013-12-04 07:51:02 UTC) #7

rune

https://codereview.chromium.org/82083002/diff/50001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/82083002/diff/50001/Source/core/dom/Document.cpp#newcode3181 Source/core/dom/Document.cpp:3181: element->setNeedsStyleRecalc(LocalStyleChange); Does LocalStyleChange work for explicit or implicit inheritance? ...

7 years ago (2013-12-04 08:27:15 UTC) #8

esprehn

On 2013/12/04 08:27:15, rune - CET wrote: > https://codereview.chromium.org/82083002/diff/50001/Source/core/dom/Document.cpp > File Source/core/dom/Document.cpp (right): > > ...

7 years ago (2013-12-04 14:25:23 UTC) #9

Timothy Loh

Sorry for the slow response. I've addressed the outstanding issues below. I've changed the magic ...

7 years ago (2013-12-15 23:41:18 UTC) #10

Sorry for the slow response. I've addressed the outstanding issues below.

I've changed the magic Length constants to be enums. Adding more enum values to
Length is a bit probably something we don't want to do in general, but perhaps
we can work out in the future how to make this nicer.

I don't want to leave viewport units unresolved for the @viewport/meta-tag; in
particular we'd still want these ViewportLength objects to support calcs of
px/%/vw/etc., and it'd probably end up as duplicated logic (or excessive
templating). A later patch I intend to write will simplify the expression trees
of platform calcs to just two floats (px + %) and having to support viewport
units in calcs reduces the amount of simplification we can do here.

https://codereview.chromium.org/82083002/diff/70001/Source/core/css/resolver/...
File Source/core/css/resolver/ViewportStyleResolver.cpp (right):

https://codereview.chromium.org/82083002/diff/70001/Source/core/css/resolver/...
Source/core/css/resolver/ViewportStyleResolver.cpp:187: return
primitiveValue->computeLength<Length>(CSSToLengthConversionData(m_document->renderStyle(),
m_document->renderStyle(), m_document->renderView(), 1.0f));
On 2013/12/04 07:44:06, rune - CET wrote:
> 
> This change means also viewport descriptors need to be recascaded on resize. I
> can't see from the code changes that this happens.
> 
> An example where the viewport should change on resize:
> 
> @viewport { width: 50vw; }

Updated patch to re-resolve on resize.

https://codereview.chromium.org/82083002/diff/70001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/82083002/diff/70001/Source/core/dom/Document....
Source/core/dom/Document.cpp:3182: }
On 2013/12/04 06:02:28, esprehn wrote:
> This doesn't handle Shadow DOM, you probably want a method on TreeScope that
you
> call recursively that notifyResizeForViewportUnits() calls.
> 
> TreeScope::setNeedsStyleRecalcForViewportUnits() {
>   for (Element ... ) {
>     for (ShadowRoot* root = element->youngestShadowRoot(); root; root =
> root->olderShadowRoot())
>      root->setNeedsStyleRecalcForViewportUnits();
>      ...
>      element->setNeedsStyleRecalc(...);
>   }
> }

Done. Also updated this to iterate over pseudoelements too and added to test.

kenneth.r.christiansen

https://codereview.chromium.org/82083002/diff/230001/LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html File LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html (right): https://codereview.chromium.org/82083002/diff/230001/LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html#newcode172 LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html:172: shouldBe('style.getPropertyValue("height")', vw(30)); Nice clearup https://codereview.chromium.org/82083002/diff/230001/LayoutTests/fast/css/computed-offset-with-zoom.html File LayoutTests/fast/css/computed-offset-with-zoom.html (right): https://codereview.chromium.org/82083002/diff/230001/LayoutTests/fast/css/computed-offset-with-zoom.html#newcode54 ...

7 years ago (2013-12-16 10:08:18 UTC) #11

Timothy Loh

https://codereview.chromium.org/82083002/diff/230001/LayoutTests/fast/css/computed-offset-with-zoom.html File LayoutTests/fast/css/computed-offset-with-zoom.html (right): https://codereview.chromium.org/82083002/diff/230001/LayoutTests/fast/css/computed-offset-with-zoom.html#newcode54 LayoutTests/fast/css/computed-offset-with-zoom.html:54: var windowWidth = "'" + (window.innerWidth / 10) + ...

7 years ago (2013-12-17 05:09:13 UTC) #12

Timothy Loh

7 years ago (2013-12-17 22:57:59 UTC) #13

esprehn

I think you broke something, now getComputedStyle(e).lineHeight sometimes returns "normal" instead of a computed value. ...

7 years ago (2013-12-18 01:03:54 UTC) #14

Timothy Loh

https://codereview.chromium.org/82083002/diff/290001/LayoutTests/platform/win/virtual/legacy-animations-engine/animations/interpolation/line-height-interpolation-expected.txt File LayoutTests/platform/win/virtual/legacy-animations-engine/animations/interpolation/line-height-interpolation-expected.txt (right): https://codereview.chromium.org/82083002/diff/290001/LayoutTests/platform/win/virtual/legacy-animations-engine/animations/interpolation/line-height-interpolation-expected.txt#newcode81 LayoutTests/platform/win/virtual/legacy-animations-engine/animations/interpolation/line-height-interpolation-expected.txt:81: FAIL: line-height from [75vw] to [25vh] was [normal] at ...

7 years ago (2013-12-18 01:54:42 UTC) #15

https://codereview.chromium.org/82083002/diff/290001/LayoutTests/platform/win...
File
LayoutTests/platform/win/virtual/legacy-animations-engine/animations/interpolation/line-height-interpolation-expected.txt
(right):

https://codereview.chromium.org/82083002/diff/290001/LayoutTests/platform/win...
LayoutTests/platform/win/virtual/legacy-animations-engine/animations/interpolation/line-height-interpolation-expected.txt:81:
FAIL: line-height from [75vw] to [25vh] was [normal] at 1.5, expected [0px]
On 2013/12/18 01:03:55, esprehn wrote:
> Did you break this?

The legacy animations engine does some ridiculous things when animating
line-height (see the rest of the expectations in the file), in particular when
values get to zero and below strange things happen. This is basically a
progression for the common cases (interpolation fraction between 0 and 1), and
in any case it's very unlikely we'll need to flip back to the legacy engine now.

https://codereview.chromium.org/82083002/diff/290001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/82083002/diff/290001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3172:
ensureStyleResolver().notifyResizeForViewportUnits();
On 2013/12/18 01:03:55, esprehn wrote:
> Do you really need to force creation of the style resolver if it doesn't
exist?
> It seems like you should be able to do:
> 
> if (StyleResolver* resolver = styleResolver())
>   resolver->...;
> 

I think the only case where we won't end up creating a styleResolver during the
resize is if the Document has the hasViewportUnits flag set but no elements
currently use them? We want to call into this for re-resolving @viewport rules,
and I figured that the case where we wouldn't be creating a StyleResolver anyway
is unlikely enough that it's simpler to not have a separate bit for viewport
units in @viewport rules.

https://codereview.chromium.org/82083002/diff/290001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/82083002/diff/290001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.h:281: // 63 bits
On 2013/12/18 01:03:55, esprehn wrote:
> Is the +3 a mistake, or did you count and was the 60 wrong?

Yep, the previous count was incorrect. I also moved the bit added here up more
to improve packing (gcc won't split an unsigned here across 32-bit boundaries).

kenneth.r.christiansen

https://codereview.chromium.org/82083002/diff/350001/Source/core/css/resolver/ViewportStyleResolver.cpp File Source/core/css/resolver/ViewportStyleResolver.cpp (left): https://codereview.chromium.org/82083002/diff/350001/Source/core/css/resolver/ViewportStyleResolver.cpp#oldcode189 Source/core/css/resolver/ViewportStyleResolver.cpp:189: if (primitiveValue->isViewportPercentageLength()) Are we still supporting @viewport { width: ...

7 years ago (2013-12-18 07:16:17 UTC) #17

Timothy Loh

https://codereview.chromium.org/82083002/diff/230001/Source/platform/LengthFunctions.cpp File Source/platform/LengthFunctions.cpp (left): https://codereview.chromium.org/82083002/diff/230001/Source/platform/LengthFunctions.cpp#oldcode46 Source/platform/LengthFunctions.cpp:46: case ViewportPercentageWidth: On 2013/12/16 10:08:19, kenneth.r.christiansen wrote: > Ain't ...

7 years ago (2013-12-18 07:18:25 UTC) #18

kenneth.r.christiansen

On 2013/12/18 07:18:25, Timothy Loh wrote: > https://codereview.chromium.org/82083002/diff/230001/Source/platform/LengthFunctions.cpp > File Source/platform/LengthFunctions.cpp (left): > > https://codereview.chromium.org/82083002/diff/230001/Source/platform/LengthFunctions.cpp#oldcode46 ...

7 years ago (2013-12-18 07:41:11 UTC) #19

kenneth.r.christiansen

John, PDR, can you please take a look at the above comment

7 years ago (2013-12-18 07:43:16 UTC) #20

johnme

Some quick comments. https://codereview.chromium.org/82083002/diff/370001/Source/core/css/CSSLengthFunctions.cpp File Source/core/css/CSSLengthFunctions.cpp (left): https://codereview.chromium.org/82083002/diff/370001/Source/core/css/CSSLengthFunctions.cpp#oldcode110 Source/core/css/CSSLengthFunctions.cpp:110: // This method has code duplicated ...

7 years ago (2013-12-18 12:04:13 UTC) #21

rune

On 2013/12/18 07:16:17, kenneth.r.christiansen wrote: > https://codereview.chromium.org/82083002/diff/350001/Source/core/css/resolver/ViewportStyleResolver.cpp > File Source/core/css/resolver/ViewportStyleResolver.cpp (left): > > https://codereview.chromium.org/82083002/diff/350001/Source/core/css/resolver/ViewportStyleResolver.cpp#oldcode189 > ...

7 years ago (2013-12-19 11:30:49 UTC) #22

Timothy Loh

On 2013/12/19 11:30:49, rune - CET wrote: > The result changes between reloads and resizing ...

7 years ago (2013-12-19 21:17:57 UTC) #23

Timothy Loh

I've tweaked the CSSToLengthConversionData to store two floats for viewport width and height, so it ...

7 years ago (2013-12-23 06:28:15 UTC) #24

I've tweaked the CSSToLengthConversionData to store two floats for viewport
width and height, so it can get different values depending on context (in
particular @viewport rules), and clarified RenderView::viewportWidthPercent to
layoutViewportWidth. Renaming the accessors in the CSSToLengthConversionData is
not what we want here since it means the initial viewport when resolving
@viewport.

This appears to make viewport units in @viewport rules work fine, but maybe
someone will want to double-check this? (Rune?)

https://codereview.chromium.org/82083002/diff/370001/Source/core/css/CSSLengt...
File Source/core/css/CSSLengthFunctions.cpp (left):

https://codereview.chromium.org/82083002/diff/370001/Source/core/css/CSSLengt...
Source/core/css/CSSLengthFunctions.cpp:110: // This method has code duplicated
in platform/LengthFunctions.cpp.
On 2013/12/18 12:04:15, johnme wrote:
> As you've removed the duplicate here, could you update the comment in
> platform/LengthFunctions.cpp ?

Done.

https://codereview.chromium.org/82083002/diff/370001/Source/core/css/CSSToLen...
File Source/core/css/CSSToLengthConversionData.h (right):

https://codereview.chromium.org/82083002/diff/370001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.h:67: // Accessing these marks the
style as having viewport units
On 2013/12/18 12:04:15, johnme wrote:
> Do any other accessors mutate the object like this? Is there a cleaner
> alternative?

I could move marking the style out into the conversion code that uses these in
CSSPrimitiveValue::computeLengthDouble. It would probably end up with a function
here markStyleAsHavingViewportUnits (since I'd rather this class stop returning
styles and just return pixel values for ems/exs/rems), and then this wouldn't be
a whole lot different to what's going on now?

https://codereview.chromium.org/82083002/diff/370001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/82083002/diff/370001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1213: double
RenderView::viewportWidthPercent() const
On 2013/12/18 12:04:15, johnme wrote:
> 1. This doesn't seem to return a percentage?
> 
> 2. Is this the initial viewport (window width in DIPs, affected only by page
> zoom), layout viewport (also affected by @viewport and meta viewport) or
actual
> viewport (also affected by pinch zoom)? Since viewWidth uses
> FrameView::layoutSize, which is set by WebViewImpl::setFixedLayoutSize, it
seems
> that this is probably the layout viewport size.
> 
> Consider renaming this to layoutViewportWidth (similarly below, and in
> CSSToLengthConversionData).

See comment (message) above.

https://codereview.chromium.org/82083002/diff/370001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:1216: return
viewWidth(ScrollableArea::IncludeScrollbars) / scale / 100.f;
On 2013/12/18 12:04:15, johnme wrote:
> According to http://www.w3.org/TR/css3-values/#viewport-relative-lengths, vw
et
> al are relative to the size of the initial containing block, whose "size is
> affected by the presence of scrollbars on the viewport". Since the size
depends
> on the presence of scrollbars, shouldn't this be using ExcludeScrollbars
> instead?

Possibly; but that would have to be a separate patch (I'm just trying to move
resolution to style time in this patch).

https://codereview.chromium.org/82083002/diff/370001/Source/platform/Length.h
File Source/platform/Length.h (right):

https://codereview.chromium.org/82083002/diff/370001/Source/platform/Length.h...
Source/platform/Length.h:45: ExtendToZoom, DeviceWidth, DeviceHeight,
On 2013/12/18 12:04:15, johnme wrote:
> I'm probably missing something, but why did you add DeviceWidth and
DeviceHeight
> here? Shouldn't they always behave identically to 100vw and 100vh (bearing in
> mind that vw/vh behave differently in @viewport, being relative to the initial
> viewport there[1], rather than the layout viewport (ICB) like they usually
> are[2])?
> 
> [1]: http://dev.w3.org/csswg/css-device-adapt/#viewport-descriptors
> [2]: http://www.w3.org/TR/css3-values/#viewport-relative-lengths

See other comments about vw/vh meaning.

This seems like the simplest way to handle this to me. Resolving these here just
complicates the logic around resizing since we'd have to update these also. Also
calling document::setHasViewportUnits would be a bit odd here, since we don't
actually have viewport units!

rune

On 2013/12/23 06:28:15, Timothy Loh wrote: > This appears to make viewport units in @viewport ...

7 years ago (2013-12-23 14:22:41 UTC) #25

kenneth.r.christiansen

lgtm https://codereview.chromium.org/82083002/diff/450001/LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-browser-zoom-expected.txt File LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-browser-zoom-expected.txt (right): https://codereview.chromium.org/82083002/diff/450001/LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-browser-zoom-expected.txt#newcode1 LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-browser-zoom-expected.txt:1: This test checks that getComputedStyle returns pixel values ...

6 years, 11 months ago (2014-01-02 09:03:24 UTC) #27

Timothy Loh

https://codereview.chromium.org/82083002/diff/450001/LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-browser-zoom-expected.txt File LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-browser-zoom-expected.txt (right): https://codereview.chromium.org/82083002/diff/450001/LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-browser-zoom-expected.txt#newcode1 LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-browser-zoom-expected.txt:1: This test checks that getComputedStyle returns pixel values for ...

6 years, 11 months ago (2014-01-03 00:23:08 UTC) #28

https://codereview.chromium.org/82083002/diff/450001/LayoutTests/css3/viewpor...
File
LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-browser-zoom-expected.txt
(right):

https://codereview.chromium.org/82083002/diff/450001/LayoutTests/css3/viewpor...
LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-browser-zoom-expected.txt:1:
This test checks that getComputedStyle returns pixel values for viewport units
that are affected by browser zoom.
On 2014/01/02 09:03:26, kenneth.r.christiansen wrote:
> the spec (CSSOM views) call this "page zoom". We should probably stick to
these
> terms as well

Done, also renamed test accordingly.

https://codereview.chromium.org/82083002/diff/450001/LayoutTests/css3/viewpor...
File
LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-browser-zoom.html
(right):

https://codereview.chromium.org/82083002/diff/450001/LayoutTests/css3/viewpor...
LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-browser-zoom.html:11:
description('This test checks that getComputedStyle returns pixel values for
viewport units that are affected by browser zoom.');
On 2014/01/02 09:03:26, kenneth.r.christiansen wrote:
> by page zoom (aka browser zoom).

Done.

https://codereview.chromium.org/82083002/diff/450001/Source/core/animation/cs...
File Source/core/animation/css/CSSAnimatableValueFactory.cpp (right):

https://codereview.chromium.org/82083002/diff/450001/Source/core/animation/cs...
Source/core/animation/css/CSSAnimatableValueFactory.cpp:84: case DeviceWidth:
On 2014/01/02 09:03:26, kenneth.r.christiansen wrote:
> To avoid anyone abusing these should we call them MetaDeviceWidth ?

Probably doesn't matter too much right now; I want to refactor Length somewhat
in the near-ish future in a way that should make it really obvious if anyone
uses the Length enums wrongly.

https://codereview.chromium.org/82083002/diff/450001/Source/core/css/CSSToLen...
File Source/core/css/CSSToLengthConversionData.cpp (right):

https://codereview.chromium.org/82083002/diff/450001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.cpp:82:
m_style->setHasViewportUnits();
On 2014/01/02 09:03:26, kenneth.r.christiansen wrote:
> isn't there any way to assert to make sure this is true? Also if the style is
> modified to not use viewport units anymore it will still be marked as such,
no?

Not sure what your first question is. I don't think styles really get modified
after they're constructed during style resolution; we just create new ones. The
bool on Document doesn't currently get reset anywhere, but this probably isn't a
case to optimise right now.