Issue 1212893005: Add position: sticky as supported position value when CSSStickyPosition is enabled.

Issue 1212893005: Add position: sticky as supported position value when CSSStickyPosition is enabled. (Closed)

Created:
5 years, 5 months ago by flackr

Modified:
5 years, 5 months ago

Reviewers:
Ian Vollick, leviw_travelin_and_unemployed, Timothy Loh, Rick Byers

CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-paint_chromium.org, blink-reviews-rendering, blink-reviews-style_chromium.org, dglazkov+blink, dshwang, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, rwlbuis, slimming-paint-reviews_chromium.org, szager+layoutwatch_chromium.org, zoltan1

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

Target Ref:
refs/heads/master

Project:
blink

Visibility:
Public.

More Reviews

Description

Add position: sticky as supported position value when CSSStickyPosition is enabled. BUG=231752 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199029

Patch Set 1 #

Total comments: 4

Patch Set 2 : Comments and merge. #

Total comments: 8

Patch Set 3 : Merge, convert pixel to ref tests, and address comments. #

Created: 5 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+196 lines, -36 lines)			Patch
A	LayoutTests/fast/css/sticky/parsing-position-sticky.html	View	1 2	1 chunk	+24 lines, -0 lines	0 comments	Download
A +	LayoutTests/fast/css/sticky/parsing-position-sticky-expected.txt	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
A	LayoutTests/fast/css/sticky/sticky-as-positioning-container.html	View	1 2	1 chunk	+32 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css/sticky/sticky-as-positioning-container-expected.html	View	1 2	1 chunk	+27 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css/sticky/sticky-stacking-context.html	View	1 2	1 chunk	+26 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css/sticky/sticky-stacking-context-expected.html	View	1 2	1 chunk	+21 lines, -0 lines	0 comments	Download
M	Source/core/css/CSSPrimitiveValueMappings.h	View		2 chunks	+5 lines, -0 lines	0 comments	Download
M	Source/core/css/CSSValueKeywords.in	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	Source/core/css/parser/CSSParserFastPaths.cpp	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/layout/LayoutBlock.cpp	View	1 2	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/core/layout/LayoutBlockFlow.cpp	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/layout/LayoutBox.cpp	View	1 2	11 chunks	+16 lines, -16 lines	0 comments	Download
M	Source/core/layout/LayoutBoxModelObject.cpp	View	1 2	2 chunks	+3 lines, -3 lines	0 comments	Download
M	Source/core/layout/LayoutInline.h	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/layout/LayoutInline.cpp	View	1 2	3 chunks	+4 lines, -4 lines	0 comments	Download
M	Source/core/layout/LayoutObject.h	View	1 2	3 chunks	+5 lines, -0 lines	0 comments	Download
M	Source/core/layout/LayoutObject.cpp	View	1 2	2 chunks	+3 lines, -1 line	0 comments	Download
M	Source/core/layout/TracedLayoutObject.h	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/layout/TracedLayoutObject.cpp	View	1	2 chunks	+3 lines, -0 lines	0 comments	Download
M	Source/core/paint/DeprecatedPaintLayer.cpp	View	1 2	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/core/style/ComputedStyle.h	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/style/ComputedStyleConstants.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/testing/InternalSettings.h	View	1 2	2 chunks	+2 lines, -0 lines	0 comments	Download
M	Source/core/testing/InternalSettings.cpp	View	1 2	3 chunks	+7 lines, -0 lines	0 comments	Download
M	Source/core/testing/InternalSettings.idl	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/platform/RuntimeEnabledFeatures.in	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 27 (11 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

flackr

Hey, this is the patch for parsing position sticky and treating is as an in ...

5 years, 5 months ago (2015-06-29 21:23:50 UTC) #2

Ian Vollick

You'll certainly need a style expert to review this, but this seems straightforward to me. ...

5 years, 5 months ago (2015-06-30 17:56:32 UTC) #3

flackr

On 2015/06/30 17:56:32, vollick wrote: > You'll certainly need a style expert to review this, ...

5 years, 5 months ago (2015-06-30 18:44:48 UTC) #4

On 2015/06/30 17:56:32, vollick wrote:
> You'll certainly need a style expert to review this, but this seems
> straightforward to me.
> 
> Just to confirm, this patch is only doing the following?
> 
>  1. parsing sticky
>  2. updating code that assumed that "in flow" + "positioned" ==> "relative
> positioned" to also consider sticky.

This is correct.

> 
> Sadly though the tests you've got seem useful, without hooking the property up
> to layout, proper test coverage seems like it'll have to come later?
> 

Yes, unfortunately there's very little we can currently test until we get
positioning logic hooked up.

https://codereview.chromium.org/1212893005/diff/1/LayoutTests/fast/css/sticky...
File LayoutTests/fast/css/sticky/sticky-stacking-context.html (right):

https://codereview.chromium.org/1212893005/diff/1/LayoutTests/fast/css/sticky...
LayoutTests/fast/css/sticky/sticky-stacking-context.html:2: <html>
On 2015/06/30 17:56:32, vollick wrote:
> no need for html, body or head.

Done. Seems I need a body in the parsing test to call appendChild on.

> 
> Please also add a comment describing what this test does (I'm aware since we
> talked about it, but it will be useful for others).

Done.

> 
> (Same comments for other layout tests).

https://codereview.chromium.org/1212893005/diff/1/Source/core/layout/LayoutOb...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1212893005/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.cpp:1083: name.append(" (relatively
positioned)");
On 2015/06/30 17:56:32, vollick wrote:
> We seem to report "relative positioned" in our output (rather than
relatively),
> despite the name in code (I'm looking at this and TracedLayoutObject). Perhaps
> this should be left alone for consistency if we're not going to update both?

Oops, didn't mean to keep this change. Originally I wasn't sure if I was going
to be able to tell the difference between relative and sticky and was going to
call this in flow positioned but we need to handle relative differently in
LayoutBoxModelObject::offsetForInFlowPosition. Restored.

Ian Vollick

On 2015/06/30 at 18:44:48, flackr wrote: > On 2015/06/30 17:56:32, vollick wrote: > > You'll ...

5 years, 5 months ago (2015-06-30 21:04:34 UTC) #6

On 2015/06/30 at 18:44:48, flackr wrote:
> On 2015/06/30 17:56:32, vollick wrote:
> > You'll certainly need a style expert to review this, but this seems
> > straightforward to me.
> > 
> > Just to confirm, this patch is only doing the following?
> > 
> >  1. parsing sticky
> >  2. updating code that assumed that "in flow" + "positioned" ==> "relative
> > positioned" to also consider sticky.
> 
> This is correct.
> 
> > 
> > Sadly though the tests you've got seem useful, without hooking the property
up
> > to layout, proper test coverage seems like it'll have to come later?
> > 
> 
> Yes, unfortunately there's very little we can currently test until we get
positioning logic hooked up.
> 
>
https://codereview.chromium.org/1212893005/diff/1/LayoutTests/fast/css/sticky...
> File LayoutTests/fast/css/sticky/sticky-stacking-context.html (right):
> 
>
https://codereview.chromium.org/1212893005/diff/1/LayoutTests/fast/css/sticky...
> LayoutTests/fast/css/sticky/sticky-stacking-context.html:2: <html>
> On 2015/06/30 17:56:32, vollick wrote:
> > no need for html, body or head.
> 
> Done. Seems I need a body in the parsing test to call appendChild on.
> 
> > 
> > Please also add a comment describing what this test does (I'm aware since we
> > talked about it, but it will be useful for others).
> 
> Done.
> 
> > 
> > (Same comments for other layout tests).
> 
>
https://codereview.chromium.org/1212893005/diff/1/Source/core/layout/LayoutOb...
> File Source/core/layout/LayoutObject.cpp (right):
> 
>
https://codereview.chromium.org/1212893005/diff/1/Source/core/layout/LayoutOb...
> Source/core/layout/LayoutObject.cpp:1083: name.append(" (relatively
positioned)");
> On 2015/06/30 17:56:32, vollick wrote:
> > We seem to report "relative positioned" in our output (rather than
relatively),
> > despite the name in code (I'm looking at this and TracedLayoutObject).
Perhaps
> > this should be left alone for consistency if we're not going to update both?
> 
> Oops, didn't mean to keep this change. Originally I wasn't sure if I was going
to be able to tell the difference between relative and sticky and was going to
call this in flow positioned but we need to handle relative differently in
LayoutBoxModelObject::offsetForInFlowPosition. Restored.

Thanks for the changes and clarifications.

+esprehn@

Elliott, I'm new to the style code and couldn't approve this change. Would you
be able to take a look or suggest another review, please?

esprehn

This looks reasonable to me, but I'm on vacation this week. Timloh@ can do style ...

5 years, 5 months ago (2015-06-30 21:51:54 UTC) #8

Ian Vollick

On 2015/06/30 21:51:54, esprehn wrote: > This looks reasonable to me, but I'm on vacation ...

5 years, 5 months ago (2015-07-01 11:44:04 UTC) #9

flackr

Hey Julien, it looks like Tim is not available, could you please take a look ...

5 years, 5 months ago (2015-07-02 14:03:36 UTC) #12

flackr

Hey Rick, you mentioned that you might have time to take a look at this. ...

5 years, 5 months ago (2015-07-03 01:17:57 UTC) #14

Rick Byers

I'm no style expert, but it seems pretty straight forward to me. Let's give the ...

5 years, 5 months ago (2015-07-03 01:43:55 UTC) #15

leviw_travelin_and_unemployed

Only looked at the layout/ code, but it lgtm.

5 years, 5 months ago (2015-07-07 22:16:05 UTC) #17

flackr

https://codereview.chromium.org/1212893005/diff/20001/LayoutTests/fast/css/sticky/parsing-position-sticky.html File LayoutTests/fast/css/sticky/parsing-position-sticky.html (right): https://codereview.chromium.org/1212893005/diff/20001/LayoutTests/fast/css/sticky/parsing-position-sticky.html#newcode1 LayoutTests/fast/css/sticky/parsing-position-sticky.html:1: <body> On 2015/07/03 01:43:55, Rick Byers (Out until 7-23) ...

5 years, 5 months ago (2015-07-14 15:24:35 UTC) #18

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1212893005/40001

5 years, 5 months ago (2015-07-16 00:41:27 UTC) #22

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/builds/41514)

5 years, 5 months ago (2015-07-16 00:49:23 UTC) #24

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1212893005/40001

5 years, 5 months ago (2015-07-16 12:13:59 UTC) #26

commit-bot: I haz the power

5 years, 5 months ago (2015-07-16 12:29:50 UTC) #27

Message was sent while issue was closed.

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199029

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages