New issue
Advanced search Search tips

Issue 849055 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 2
Type: Bug

Blocking:
issue 848171



Sign in to add a comment

Calculate event-dependent NavigationPolicy before calling into FrameLoader

Project Member Reported by dgozman@chromium.org, Jun 2 2018

Issue description

- This will simplify FrameLoader, and make it obvious where new foreground/background window/popup is determined.
- We can then remove TriggeringEvent from FrameLoadRequest.
- This logic can go as well: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/exported/local_frame_client_impl.cc?rcl=363d153b234835ab85f648015bf4622677b31bd2&l=472
 
Blocking: 848171
Labels: OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Windows
Note that ctrl-clicking submit button on a form does not open new background tab. This is because form navigation is scheduled asynchronously so that check for AllowCreatingBackgroundTabs() does not have synchronous CurrentInputEvent.

Safari does handle this correctly.
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9e6471cbcd8905ab142a1cac1089de5029f414ba

commit 9e6471cbcd8905ab142a1cac1089de5029f414ba
Author: Dmitry Gozman <dgozman@chromium.org>
Date: Wed Jun 13 20:23:39 2018

FrameLoader: remove FrameLoadRequest::TriggeringEvent

The event is only used by default click handlers and form submission.
This patch replaces event with NavigationPolicy calculated
on the call sites and (unfortunately) WebEventTriggeringInfo.

Replaced one unit test with layout test which exercises real
click instead of simulating it.

Next patch will encapsulate all navigation policy logic in
navigation_policy.cc and fix a bug with form submissions ignoring
event modifiers (see issue).

TBR=haraken@chromium.org

Bug:  849055 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ic64b0eec02d69e58d0dbe283abd7ee157f46f747
Reviewed-on: https://chromium-review.googlesource.com/1084323
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566969}
[add] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/WebKit/LayoutTests/fast/events/new-tab-on-middle-click-expected.txt
[add] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/WebKit/LayoutTests/fast/events/new-tab-on-middle-click.html
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/bindings/core/v8/local_window_proxy.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/exported/local_frame_client_impl.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/exported/web_frame_test.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/exported/web_view_test.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/frame/local_frame_client.h
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/html/custom/custom_element_registry_test.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/html/custom/custom_element_test.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/html/forms/html_form_element.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/html/html_anchor_element.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/layout/custom/layout_worklet_test.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/layout/layout_box_model_object_test.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/loader/form_submission.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/loader/form_submission.h
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/loader/frame_load_request.h
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/loader/frame_loader.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/loader/frame_loader.h
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/loader/navigation_policy.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/loader/navigation_policy.h
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/loader/navigation_scheduler.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/page/create_window.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/page/print_context_test.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/paint/paint_and_raster_invalidation_test.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/paint/paint_layer_clipper_test.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/paint/pre_paint_tree_walk_test.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/svg/svg_a_element.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/core/timing/window_performance_test.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/modules/credentialmanager/credentials_container.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/modules/csspaint/paint_worklet_test.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/modules/media_controls/elements/media_control_input_element_test.cc
[modify] https://crrev.com/9e6471cbcd8905ab142a1cac1089de5029f414ba/third_party/blink/renderer/modules/media_controls/media_controls_rotate_to_fullscreen_delegate_test.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/323d55f784a8caeafc670cce692c1846423200d4

commit 323d55f784a8caeafc670cce692c1846423200d4
Author: Dmitry Gozman <dgozman@chromium.org>
Date: Thu Jun 14 21:21:11 2018

Move more NavigationPolicy decisions to navigation_policy.cc

As a side effect, this makes ctrl+click on a submit button open
a new background tab - similar to webkit and clicking on a link.

Also covered ctrl+click functionality with tests.

Bug:  849055 
Change-Id: I344df4821a7c840129d239023d4a500a42e8f80b
Reviewed-on: https://chromium-review.googlesource.com/1100107
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567422}
[add] https://crrev.com/323d55f784a8caeafc670cce692c1846423200d4/third_party/WebKit/LayoutTests/fast/events/background-tab-on-ctrl-click-expected.txt
[add] https://crrev.com/323d55f784a8caeafc670cce692c1846423200d4/third_party/WebKit/LayoutTests/fast/events/background-tab-on-ctrl-click.html
[add] https://crrev.com/323d55f784a8caeafc670cce692c1846423200d4/third_party/WebKit/LayoutTests/fast/events/background-tab-on-submit-ctrl-click-expected.txt
[add] https://crrev.com/323d55f784a8caeafc670cce692c1846423200d4/third_party/WebKit/LayoutTests/fast/events/background-tab-on-submit-ctrl-click.html
[add] https://crrev.com/323d55f784a8caeafc670cce692c1846423200d4/third_party/WebKit/LayoutTests/fast/events/background-tab-on-submit-synthesized-ctrl-click-expected.txt
[add] https://crrev.com/323d55f784a8caeafc670cce692c1846423200d4/third_party/WebKit/LayoutTests/fast/events/background-tab-on-submit-synthesized-ctrl-click.html
[add] https://crrev.com/323d55f784a8caeafc670cce692c1846423200d4/third_party/WebKit/LayoutTests/fast/events/background-tab-on-synthesized-ctrl-click-expected.txt
[add] https://crrev.com/323d55f784a8caeafc670cce692c1846423200d4/third_party/WebKit/LayoutTests/fast/events/background-tab-on-synthesized-ctrl-click.html
[modify] https://crrev.com/323d55f784a8caeafc670cce692c1846423200d4/third_party/blink/renderer/core/exported/local_frame_client_impl.cc
[modify] https://crrev.com/323d55f784a8caeafc670cce692c1846423200d4/third_party/blink/renderer/core/loader/navigation_policy.cc
[modify] https://crrev.com/323d55f784a8caeafc670cce692c1846423200d4/third_party/blink/renderer/core/loader/navigation_policy.h
[modify] https://crrev.com/323d55f784a8caeafc670cce692c1846423200d4/third_party/blink/renderer/core/page/create_window.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc43df3b38cf52d2a36f80ee56ae1cd997135dbb

commit fc43df3b38cf52d2a36f80ee56ae1cd997135dbb
Author: Dmitry Gozman <dgozman@chromium.org>
Date: Fri Jun 15 13:05:56 2018

Simplify EffectiveNavigationPolicy and friends

We always pass kNavigationPolicyCurrentTab from FrameLoader,
which is turned into kNavigationPolicyNewForegroundTab.

This observation allows us to remove dead branches in
EffectiveNavigationPolicy and simplify some calls there.

Bug:  849055 
Change-Id: I9d1c08fba784111f3a544685da3f69c69380bedc
Reviewed-on: https://chromium-review.googlesource.com/1100243
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567618}
[modify] https://crrev.com/fc43df3b38cf52d2a36f80ee56ae1cd997135dbb/third_party/blink/renderer/core/loader/frame_loader.cc
[modify] https://crrev.com/fc43df3b38cf52d2a36f80ee56ae1cd997135dbb/third_party/blink/renderer/core/page/create_window.cc
[modify] https://crrev.com/fc43df3b38cf52d2a36f80ee56ae1cd997135dbb/third_party/blink/renderer/core/page/create_window.h
[modify] https://crrev.com/fc43df3b38cf52d2a36f80ee56ae1cd997135dbb/third_party/blink/renderer/core/page/effective_navigation_policy_test.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9df221c222ba8738ad557b4b0493781c4d6212f8

commit 9df221c222ba8738ad557b4b0493781c4d6212f8
Author: Dmitry Gozman <dgozman@chromium.org>
Date: Fri Jun 15 15:48:29 2018

Move last NavigationPolicy decisions to navigation_policy.cc

Bug:  849055 
Change-Id: Ie60a938bc8831d149cd13c796e0089a5c3ccd988
Reviewed-on: https://chromium-review.googlesource.com/1100309
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567657}
[modify] https://crrev.com/9df221c222ba8738ad557b4b0493781c4d6212f8/third_party/blink/renderer/core/BUILD.gn
[modify] https://crrev.com/9df221c222ba8738ad557b4b0493781c4d6212f8/third_party/blink/renderer/core/events/current_input_event.h
[modify] https://crrev.com/9df221c222ba8738ad557b4b0493781c4d6212f8/third_party/blink/renderer/core/loader/navigation_policy.cc
[modify] https://crrev.com/9df221c222ba8738ad557b4b0493781c4d6212f8/third_party/blink/renderer/core/loader/navigation_policy.h
[rename] https://crrev.com/9df221c222ba8738ad557b4b0493781c4d6212f8/third_party/blink/renderer/core/loader/navigation_policy_test.cc
[modify] https://crrev.com/9df221c222ba8738ad557b4b0493781c4d6212f8/third_party/blink/renderer/core/page/create_window.cc
[modify] https://crrev.com/9df221c222ba8738ad557b4b0493781c4d6212f8/third_party/blink/renderer/core/page/create_window.h

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/13cef1e7a622c4fa13e0a9bdbb4888948db25e48

commit 13cef1e7a622c4fa13e0a9bdbb4888948db25e48
Author: Dmitry Gozman <dgozman@chromium.org>
Date: Sat Jun 16 00:24:12 2018

Add more tests for NavigationPolicy decisions

Bug:  849055 
Change-Id: If18969f45020b6dfb559c73603724854661e8508
Reviewed-on: https://chromium-review.googlesource.com/1102559
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567836}
[modify] https://crrev.com/13cef1e7a622c4fa13e0a9bdbb4888948db25e48/third_party/blink/renderer/core/loader/navigation_policy_test.cc

Status: Fixed (was: Assigned)

Sign in to add a comment