Clear hash when setting URL in document.open

Clear hash when setting URL in document.open()

A document created via document.open()/write()/close() will have
as its URL the "responsible document" (see step #23 of
https://html.spec.whatwg.org/multipage/webappapis.html#dom-document-open)

This is a problem when the URL includes a fragment identifier, any
frame with a document.open from a document with a fragment will try
to scroll the iframe into view. I believe the intent in the spec was
not to include the hash.

BUG=699267
Review-Url: https://codereview.chromium.org/2801093002
Cr-Commit-Position: refs/heads/master@{#466770}
Committed: https://chromium.googlesource.com/chromium/src/+/783d491b68d1b5833b65accba2e4fd595f09a404

[bokan, 2017-04-06 16:13:28]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 16:33:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 18:08:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 18:08:42]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[bokan, 2017-04-06 20:45:17]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 20:46:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2017-04-06 22:18:41]

Description was changed from

==========
Make fragment anchor navigations be non-recursive

Navigating to a fragment anchor (e.g. example.com/index.html#section)
will find the node named by the fragment and scroll it into view. This
will recursively scroll sub-scrollers and iframes so that the anchor is
in view on navigation.

An empty fragment ('#') or '#top' have a special meaning: scroll to top.
We previously did this by scrolling the document node into view. This
breaks when an iframe tries to scroll the fragment because it'll try to
recursively scroll its document into view. For these special cases, we
simply reset the scroll offset of the current frame.

BUG=699267
==========

to

==========
Make fragment anchor navigations be non-recursive

Navigating to a fragment anchor (e.g. example.com/index.html#section)
will find the node named by the fragment and scroll it into view. This
will recursively scroll sub-scrollers and iframes so that the anchor is
in view on navigation.

An empty fragment ('#') or '#top' have a special meaning: scroll to top.
We previously did this by scrolling the document node into view. This
breaks when an iframe tries to scroll the fragment because it'll try to
recursively scroll its document into view. For these special cases, we
simply reset the scroll offset of the current frame.

BUG=699267
==========

[bokan, 2017-04-06 22:18:42]

bokan@chromium.org changed reviewers:
+ skobes@chromium.org

[commit-bot: I haz the power, 2017-04-06 22:25:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 22:25:17]

Dry run: This issue passed the CQ dry run.

[skobes, 2017-04-11 20:15:02]

I'm confused.  Doesn't an iframe have its own URL and hash fragment, independent
of other frames?  Why would a fragment in the top-level frame cause the iframe
to scroll?

[bokan, 2017-04-12 12:50:44]

On 2017/04/11 20:15:02, skobes wrote:
> I'm confused.  Doesn't an iframe have its own URL and hash fragment,
independent
> of other frames?  Why would a fragment in the top-level frame cause the iframe
> to scroll?

Hmm, we currently do today so I assumed that was intended but I checked Firefox
and their behavior is different, so I think you may be right - the bug might be
that we're leaking the top level hash into iframes. I'll investigate a bit more.

[bokan, 2017-04-13 13:22:14]

On 2017/04/12 12:50:44, bokan wrote:
> On 2017/04/11 20:15:02, skobes wrote:
> > I'm confused.  Doesn't an iframe have its own URL and hash fragment,
> independent
> > of other frames?  Why would a fragment in the top-level frame cause the
iframe
> > to scroll?
> 
> Hmm, we currently do today so I assumed that was intended but I checked
Firefox
> and their behavior is different, so I think you may be right - the bug might
be
> that we're leaking the top level hash into iframes. I'll investigate a bit
more.

Ok, I figured out what's going on. The test case in the bug creates the iframe
by doing a document.open() and writing to it. My read of the HTML5 spec
(https://html.spec.whatwg.org/multipage/webappapis.html#dom-document-open step
#23) is in this case the iframe get's the creating document's URL which includes
the hash and this is in fact what Chrome does. According to the spec we should
use the "scroll into view" algorithm from CSSOM View which says to recursively
scroll into view any viewports
(https://drafts.csswg.org/cssom-view/#scroll-an-element-into-view). Firefox
doesn't do the recursive part so that's where the difference comes in.

According to the spec, an empty hash (and I guess we alias #top to that) is
special only in that it uses the top of the document as the target. So I think
our behavior is technically correct - though it does feel a little weird in this
case. I think a special case for "#top and #" where it doesn't recurse would
make sense but I also don't think this is serious enough to warrant special
behavior. Shall I close as WAI in that case?

[skobes, 2017-04-14 22:20:43]

On 2017/04/13 13:22:14, bokan wrote:
> Ok, I figured out what's going on. The test case in the bug creates the iframe
> by doing a document.open() and writing to it. My read of the HTML5 spec
> (https://html.spec.whatwg.org/multipage/webappapis.html#dom-document-open step
> #23) is in this case the iframe get's the creating document's URL which
includes
> the hash and this is in fact what Chrome does.

Does this mean it's impossible for a page with a hash fragment to write into an
iframe without causing scrolling?  That seems bad.

Despite the spec, I don't see how it makes sense for document.open() to inherit
the hash fragment of the execution context.  Hash-change navigations don't look
inside iframes, and https://html.spec.whatwg.org/#scroll-to-fragid refers only
to a single document.  In general each document is like a unique namespace for
its anchors.  If a website wants this behavior it can copy the location.hash
explicitly.

Do you think it's feasible for document.open() to exclude the hash fragment when
it sets the URL?

[bokan, 2017-04-18 19:47:39]

On 2017/04/14 22:20:43, skobes wrote:
> On 2017/04/13 13:22:14, bokan wrote:
> > Ok, I figured out what's going on. The test case in the bug creates the
iframe
> > by doing a document.open() and writing to it. My read of the HTML5 spec
> > (https://html.spec.whatwg.org/multipage/webappapis.html#dom-document-open
step
> > #23) is in this case the iframe get's the creating document's URL which
> includes
> > the hash and this is in fact what Chrome does.
> 
> Does this mean it's impossible for a page with a hash fragment to write into
an
> iframe without causing scrolling?  That seems bad.
> 
> Despite the spec, I don't see how it makes sense for document.open() to
inherit
> the hash fragment of the execution context.  Hash-change navigations don't
look
> inside iframes, and https://html.spec.whatwg.org/#scroll-to-fragid refers only
> to a single document.  In general each document is like a unique namespace for
> its anchors.  If a website wants this behavior it can copy the location.hash
> explicitly.
> 
> Do you think it's feasible for document.open() to exclude the hash fragment
when
> it sets the URL?

Good point, this does make document.open rather impractical to use. I think it'd
make sense to exclude the hash in this case. Firefox just sets the document
location to about:blank. I'll see if I can do that instead.

[bokan, 2017-04-18 21:22:18]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 21:22:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2017-04-18 21:26:28]

Description was changed from

==========
Make fragment anchor navigations be non-recursive

Navigating to a fragment anchor (e.g. example.com/index.html#section)
will find the node named by the fragment and scroll it into view. This
will recursively scroll sub-scrollers and iframes so that the anchor is
in view on navigation.

An empty fragment ('#') or '#top' have a special meaning: scroll to top.
We previously did this by scrolling the document node into view. This
breaks when an iframe tries to scroll the fragment because it'll try to
recursively scroll its document into view. For these special cases, we
simply reset the scroll offset of the current frame.

BUG=699267
==========

to

==========
Clear hash when setting URL in document.open()

A document created via document.open()/write()/close() will have
as its URL the "responsible document" (see step #23 of
https://html.spec.whatwg.org/multipage/webappapis.html#dom-document-open)

This is a problem when the URL includes a fragment identifier, any
frame with a document.open from a document with a fragment will try
to scroll the iframe into view. I believe the intent in the spec was
not to include the hash.

BUG=699267
==========

[commit-bot: I haz the power, 2017-04-18 23:11:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-18 23:11:29]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-04-21 15:59:38]

On 2017/04/18 19:47:39, bokan wrote:
> On 2017/04/14 22:20:43, skobes wrote:
> > On 2017/04/13 13:22:14, bokan wrote:
> > > Ok, I figured out what's going on. The test case in the bug creates the
> iframe
> > > by doing a document.open() and writing to it. My read of the HTML5 spec
> > > (https://html.spec.whatwg.org/multipage/webappapis.html#dom-document-open
> step
> > > #23) is in this case the iframe get's the creating document's URL which
> > includes
> > > the hash and this is in fact what Chrome does.
> > 
> > Does this mean it's impossible for a page with a hash fragment to write into
> an
> > iframe without causing scrolling?  That seems bad.
> > 
> > Despite the spec, I don't see how it makes sense for document.open() to
> inherit
> > the hash fragment of the execution context.  Hash-change navigations don't
> look
> > inside iframes, and https://html.spec.whatwg.org/#scroll-to-fragid refers
only
> > to a single document.  In general each document is like a unique namespace
for
> > its anchors.  If a website wants this behavior it can copy the location.hash
> > explicitly.
> > 
> > Do you think it's feasible for document.open() to exclude the hash fragment
> when
> > it sets the URL?
> 
> Good point, this does make document.open rather impractical to use. I think
it'd
> make sense to exclude the hash in this case. Firefox just sets the document
> location to about:blank. I'll see if I can do that instead.

So Firefox sets the location to about:blank but interestingly, the document.URL
and document.baseURI also include the fragment. In fact, Firefox does scroll the
fragment in the frame into view, but they don't recursively scroll into view
fragments so the main frame isn't scrolled down.

According to Anne in
https://github.com/whatwg/html/issues/2555#issuecomment-295133059, a fragment
scroll should only happen on navigation -- which document.open isn't -- so it
seems like that's the real bug. I'll see what he thinks about just omitting the
hash in the URL for document.open as that seems like an easy thing to do and the
subframe shouldn't ever want the hash anyway.

[bokan, 2017-04-21 16:03:34]

Also, see b/24408263 (Google internal) for a case where this is causing issues.
We may want to land this at least as a temporary measure to prevent wider
breakage while we fix the underlying issue.

[skobes, 2017-04-21 17:19:57]

I'm not sure it's better for document.open() to set the hash without causing a
scroll, but I could be convinced.  Anyway I think we can land this approach for
now.

https://codereview.chromium.org/2801093002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2801093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2731: // Clear the hash fragment
from the inerited URL to prevent a
typo: inherited

https://codereview.chromium.org/2801093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2736: SetURL(new_url);
Does this code run only for cross-frame document.open(), or also for
document.open() on the current frame?  We probably only want to do this in the
cross-frame case.

[bokan, 2017-04-24 16:20:42]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 16:21:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2017-04-24 17:39:04]

https://codereview.chromium.org/2801093002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2801093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2731: // Clear the hash fragment
from the inerited URL to prevent a
On 2017/04/21 17:19:56, skobes wrote:
> typo: inherited

Done.

https://codereview.chromium.org/2801093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2736: SetURL(new_url);
On 2017/04/21 17:19:56, skobes wrote:
> Does this code run only for cross-frame document.open(), or also for
> document.open() on the current frame?  We probably only want to do this in the
> cross-frame case.

Currently this was for any frame. I've changed it to run only when opened from a
cross-frame document.

[commit-bot: I haz the power, 2017-04-24 17:48:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-24 17:48:55]

Dry run: This issue passed the CQ dry run.

[skobes, 2017-04-24 18:21:14]

lgtm

[bokan, 2017-04-24 21:13:20]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2017-04-24 21:13:56]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 21:21:46]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493068400146530,
"parent_rev": "d05d1e844e7d376400a7437989c4c50a5875b36f", "commit_rev":
"783d491b68d1b5833b65accba2e4fd595f09a404"}

[commit-bot: I haz the power, 2017-04-24 21:21:58]

Description was changed from

==========
Clear hash when setting URL in document.open()

A document created via document.open()/write()/close() will have
as its URL the "responsible document" (see step #23 of
https://html.spec.whatwg.org/multipage/webappapis.html#dom-document-open)

This is a problem when the URL includes a fragment identifier, any
frame with a document.open from a document with a fragment will try
to scroll the iframe into view. I believe the intent in the spec was
not to include the hash.

BUG=699267
==========

to

==========
Clear hash when setting URL in document.open()

A document created via document.open()/write()/close() will have
as its URL the "responsible document" (see step #23 of
https://html.spec.whatwg.org/multipage/webappapis.html#dom-document-open)

This is a problem when the URL includes a fragment identifier, any
frame with a document.open from a document with a fragment will try
to scroll the iframe into view. I believe the intent in the spec was
not to include the hash.

BUG=699267

Review-Url: https://codereview.chromium.org/2801093002
Cr-Commit-Position: refs/heads/master@{#466770}
Committed:
https://chromium.googlesource.com/chromium/src/+/783d491b68d1b5833b65accba2e4...
==========

[commit-bot: I haz the power, 2017-04-24 21:21:59]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/783d491b68d1b5833b65accba2e4...