Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(260)

Issue 732783002: Reland HTMLPlugInElement: Use custom focus logic only when there is a plugin. (Closed)

Created:
6 years, 1 month ago by jbroman
Modified:
6 years, 1 month ago
Reviewers:
Mike West, *Rick Byers
CC:
blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org
Project:
blink
Visibility:
Public.

Description

Reland HTMLPlugInElement: Use custom focus logic only when there is a plugin. This is a reland of https://codereview.chromium.org/717193003/ which was reverted due to a SyzyASAN failure: https://code.google.com/p/chromium/issues/detail?id=433357 I'm not quite sure how my patch caused the issue, but reverting it did fix it. It seems that layout was being triggered during isKeyboardFocusable(), during which the plugin element itself was destroyed. This changes that path to only use an existing plugin to check for keyboard focusability, similar to other code in HTMLPlugInElement. TEST=fast/plugins/plugin/placeholder-focus.html BUG=364716 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185468

Patch Set 1 #

Patch Set 2 : form value access immediate (plugins/form-value.html) #

Patch Set 3 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+74 lines, -7 lines) Patch
A LayoutTests/fast/plugins/plugin-placeholder-focus.html View 1 chunk +27 lines, -0 lines 0 comments Download
A LayoutTests/fast/plugins/plugin-placeholder-focus-expected.txt View 1 chunk +14 lines, -0 lines 0 comments Download
M Source/core/html/HTMLObjectElement.cpp View 1 1 chunk +3 lines, -1 line 0 comments Download
M Source/core/html/HTMLPlugInElement.h View 1 chunk +9 lines, -1 line 0 comments Download
M Source/core/html/HTMLPlugInElement.cpp View 4 chunks +21 lines, -5 lines 0 comments Download

Messages

Total messages: 9 (3 generated)
jbroman
+mkwst (reviewer on original change) +rbyers (who added existingRenderWidget/renderWidgetForJSBindings previously to address a similar issue) ...
6 years, 1 month ago (2014-11-17 17:42:55 UTC) #2
Mike West
Happy to lgtm-stamp this, but please wait for Rick's feedback before landing.
6 years, 1 month ago (2014-11-17 20:00:03 UTC) #4
Rick Byers
Seems like a reasonable extension of the pattern I introduced in http://crrev.com/23093019. LGTM
6 years, 1 month ago (2014-11-17 22:18:06 UTC) #5
jbroman
On 2014/11/17 22:18:06, Rick Byers wrote: > Seems like a reasonable extension of the pattern ...
6 years, 1 month ago (2014-11-17 22:20:42 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/732783002/40001
6 years, 1 month ago (2014-11-17 22:21:47 UTC) #8
commit-bot: I haz the power
6 years, 1 month ago (2014-11-17 22:28:04 UTC) #9
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as 185468

Powered by Google App Engine
This is Rietveld 408576698