Issue 1075393002: Implement Document.scrollingElement API behind experimental feature flag

Issue 1075393002: Implement Document.scrollingElement API behind experimental feature flag (Closed)

Created:
5 years, 8 months ago by Rick Byers

Modified:
5 years, 8 months ago

Reviewers:
Ian Vollick, tdresser

CC:
blink-reviews, blink-reviews-html_chromium.org, arv+blink, vivekg, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, vivekg_samsung, Inactive, rwlbuis

Base URL:
svn://svn.chromium.org/blink/trunk

Target Ref:
refs/heads/master

Project:
blink

Visibility:
Public.

More Reviews

Description

Implement Document.scrollingElement API behind experimental feature flag Intent to implement thread: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/mJ5SlCj3CHc Also re-implement Element scroll APIs using scrollingElement to be much simpler (without changing behavior). Also corrects the behavior of Element.scrollHeight and scrollWidth to be spec compliant when ScrollTopLeftInterop mode is enabled. BUG=475531, 469835 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193662

Patch Set 1 #

Total comments: 2

Patch Set 2 : Put scrollingElement behind a flag for now #

Total comments: 6

Patch Set 3 : tdresser cr feedback #

Patch Set 4 : Minor fix and comment tweak #

Created: 5 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+228 lines, -374 lines)			Patch
M	LayoutTests/fast/dom/Element/resources/scrollable-iframe-quirks.html	View	1 2	1 chunk	+8 lines, -2 lines	0 comments	Download
M	LayoutTests/fast/dom/Element/resources/scrollable-iframe-strict.html	View		1 chunk	+6 lines, -2 lines	0 comments	Download
M	LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html	View	1 2	1 chunk	+81 lines, -77 lines	0 comments	Download
M	LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body-expected.txt	View	1 2	1 chunk	+12 lines, -4 lines	0 comments	Download
M	LayoutTests/fast/dom/Element/scrollTop-scrollLeft-strict-quirks-modes.html	View	1 2	1 chunk	+45 lines, -35 lines	0 comments	Download
M	LayoutTests/fast/dom/Element/scrollTop-scrollLeft-strict-quirks-modes-expected.txt	View	1 2	1 chunk	+16 lines, -4 lines	0 comments	Download
M	LayoutTests/webexposed/global-interface-listing-expected.txt	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/dom/Document.h	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/dom/Document.cpp	View	1 2 3	1 chunk	+17 lines, -0 lines	0 comments	Download
M	Source/core/dom/Document.idl	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/dom/Element.cpp	View	1 2	7 chunks	+38 lines, -56 lines	0 comments	Download
M	Source/core/frame/FrameView.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/HTMLBodyElement.h	View		1 chunk	+0 lines, -12 lines	0 comments	Download
M	Source/core/html/HTMLBodyElement.cpp	View		2 chunks	+0 lines, -181 lines	0 comments	Download
M	Source/platform/RuntimeEnabledFeatures.in	View	1	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 18 (6 generated)

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

Rick Byers

I see the test changes I made are hard to discern in rietveld because of ...

5 years, 8 months ago (2015-04-10 21:25:18 UTC) #3

tdresser

Some test failures look related, can you take a look? https://codereview.chromium.org/1075393002/diff/1/LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html File LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html (right): https://codereview.chromium.org/1075393002/diff/1/LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html#newcode24 ...

5 years, 8 months ago (2015-04-13 13:45:30 UTC) #4

Rick Byers

Thanks Tim. https://codereview.chromium.org/1075393002/diff/1/LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html File LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html (right): https://codereview.chromium.org/1075393002/diff/1/LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html#newcode24 LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html:24: description('Test for scrollTop/Left and scollingElement values of ...

5 years, 8 months ago (2015-04-13 20:08:31 UTC) #5

Thanks Tim.

https://codereview.chromium.org/1075393002/diff/1/LayoutTests/fast/dom/Elemen...
File LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html (right):

https://codereview.chromium.org/1075393002/diff/1/LayoutTests/fast/dom/Elemen...
LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html:24:
description('Test for scrollTop/Left and scollingElement values of scrollable
document.body in quirks and strict modes');
On 2015/04/13 13:45:30, tdresser wrote:
> scollingElement -> scrollingElement

Done.

https://codereview.chromium.org/1075393002/diff/20001/LayoutTests/fast/dom/El...
File LayoutTests/fast/dom/Element/resources/scrollable-iframe-quirks.html
(right):

https://codereview.chromium.org/1075393002/diff/20001/LayoutTests/fast/dom/El...
LayoutTests/fast/dom/Element/resources/scrollable-iframe-quirks.html:5: padding:
5px;
On 2015/04/13 13:45:30, tdresser wrote:
> Why is this section necessary? Can both margin and padding be set to 0? Or is
> this the only test looking at scrollWidth / scrollHeight?

Right, scrollTop-scrollLeft-strict-quirks-modes.html is testing the behavior
change of scrollWidth/scrollHeight with quriks mode.  I've added a comment to
help make this clear.

https://codereview.chromium.org/1075393002/diff/20001/Source/core/dom/Element...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1075393002/diff/20001/Source/core/dom/Element...
Source/core/dom/Element.cpp:716: return window->scrollX();
On 2015/04/13 13:45:30, tdresser wrote:
> I think these would be somewhat easier to read as:
> if (document().domWindow())
>   return document().domWindow()->scrollX();
> 
> Using assignment in a conditional makes it a little harder to read, and I
don't
> think the benefit is worth it.

Done for these cases 'window' is used only once.  For the cases like 'box' below
and setScrollTop/Left uses of 'window' we need to use the local twice and it
seems redundant to repeat the same expression three times in two lines.  WDYT?

I've had reviewers ask me to use this style before, but I can't find anything in
any of the style guides so perhaps it's just reviewer preference...

https://codereview.chromium.org/1075393002/diff/20001/Source/core/html/HTMLBo...
File Source/core/html/HTMLBodyElement.cpp (left):

https://codereview.chromium.org/1075393002/diff/20001/Source/core/html/HTMLBo...
Source/core/html/HTMLBodyElement.cpp:255: // FIXME: Investigate how are other
browsers casting to int (rounding, ceiling, ...).
On 2015/04/13 13:45:30, tdresser wrote:
> Is our behavior here correct at this point?

I removed the special casing of body in this regard and made it use the common
code path used by all elements which is designed for LayoutUnit - so no casting
to int at all here.  I suspect this special-case code escaped our conversion to
LayoutUnits.

It's possible there's still some related issues here or (more likely) in the
other places we round to integers.  But I don't believe I'm changing the
behavior and I don't think any new FIXMEs are warranted (we'll investigate if
and when we see a reported issue - seems unlikely to be a real problem to me).

tdresser

On 2015/04/13 20:08:31, Rick Byers wrote: > Thanks Tim. > > https://codereview.chromium.org/1075393002/diff/1/LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html > File LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html ...

5 years, 8 months ago (2015-04-13 20:30:14 UTC) #6

On 2015/04/13 20:08:31, Rick Byers wrote:
> Thanks Tim.
> 
>
https://codereview.chromium.org/1075393002/diff/1/LayoutTests/fast/dom/Elemen...
> File LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html (right):
> 
>
https://codereview.chromium.org/1075393002/diff/1/LayoutTests/fast/dom/Elemen...
> LayoutTests/fast/dom/Element/scrollTop-scrollLeft-body.html:24:
> description('Test for scrollTop/Left and scollingElement values of scrollable
> document.body in quirks and strict modes');
> On 2015/04/13 13:45:30, tdresser wrote:
> > scollingElement -> scrollingElement
> 
> Done.
> 
>
https://codereview.chromium.org/1075393002/diff/20001/LayoutTests/fast/dom/El...
> File LayoutTests/fast/dom/Element/resources/scrollable-iframe-quirks.html
> (right):
> 
>
https://codereview.chromium.org/1075393002/diff/20001/LayoutTests/fast/dom/El...
> LayoutTests/fast/dom/Element/resources/scrollable-iframe-quirks.html:5:
padding:
> 5px;
> On 2015/04/13 13:45:30, tdresser wrote:
> > Why is this section necessary? Can both margin and padding be set to 0? Or
is
> > this the only test looking at scrollWidth / scrollHeight?
> 
> Right, scrollTop-scrollLeft-strict-quirks-modes.html is testing the behavior
> change of scrollWidth/scrollHeight with quriks mode.  I've added a comment to
> help make this clear.
> 
>
https://codereview.chromium.org/1075393002/diff/20001/Source/core/dom/Element...
> File Source/core/dom/Element.cpp (right):
> 
>
https://codereview.chromium.org/1075393002/diff/20001/Source/core/dom/Element...
> Source/core/dom/Element.cpp:716: return window->scrollX();
> On 2015/04/13 13:45:30, tdresser wrote:
> > I think these would be somewhat easier to read as:
> > if (document().domWindow())
> >   return document().domWindow()->scrollX();
> > 
> > Using assignment in a conditional makes it a little harder to read, and I
> don't
> > think the benefit is worth it.
> 
> Done for these cases 'window' is used only once.  For the cases like 'box'
below
> and setScrollTop/Left uses of 'window' we need to use the local twice and it
> seems redundant to repeat the same expression three times in two lines.  WDYT?
> 
> I've had reviewers ask me to use this style before, but I can't find anything
in
> any of the style guides so perhaps it's just reviewer preference...
> 
>
https://codereview.chromium.org/1075393002/diff/20001/Source/core/html/HTMLBo...
> File Source/core/html/HTMLBodyElement.cpp (left):
> 
>
https://codereview.chromium.org/1075393002/diff/20001/Source/core/html/HTMLBo...
> Source/core/html/HTMLBodyElement.cpp:255: // FIXME: Investigate how are other
> browsers casting to int (rounding, ceiling, ...).
> On 2015/04/13 13:45:30, tdresser wrote:
> > Is our behavior here correct at this point?
> 
> I removed the special casing of body in this regard and made it use the common
> code path used by all elements which is designed for LayoutUnit - so no
casting
> to int at all here.  I suspect this special-case code escaped our conversion
to
> LayoutUnits.
> 
> It's possible there's still some related issues here or (more likely) in the
> other places we round to integers.  But I don't believe I'm changing the
> behavior and I don't think any new FIXMEs are warranted (we'll investigate if
> and when we see a reported issue - seems unlikely to be a real problem to me).

LGTM

Rick Byers

Thanks. +vollick for OWNERS in Source/platform (for trivial and temporary RuntimeEnabledFeatures addition).

5 years, 8 months ago (2015-04-13 20:36:29 UTC) #8

Ian Vollick

On 2015/04/13 20:36:29, Rick Byers wrote: > Thanks. +vollick for OWNERS in Source/platform (for trivial ...

5 years, 8 months ago (2015-04-13 20:38:17 UTC) #9

commit-bot: I haz the power

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

5 years, 8 months ago (2015-04-13 20:42:07 UTC) #11

Rick Byers

Hey Tim, I realized I really should be ensuring layout is up to date (as ...

5 years, 8 months ago (2015-04-13 21:27:31 UTC) #13

commit-bot: I haz the power

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

5 years, 8 months ago (2015-04-13 21:28:31 UTC) #16

commit-bot: I haz the power

Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=193662

5 years, 8 months ago (2015-04-14 01:27:24 UTC) #17

tdresser

5 years, 8 months ago (2015-04-14 16:14:13 UTC) #18

Message was sent while issue was closed.

On 2015/04/14 01:27:24, I haz the power (commit-bot) wrote:
> Committed patchset #4 (id:60001) as
> https://src.chromium.org/viewvc/blink?view=rev&revision=193662

LGTM

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