Bug 96148 - Web Inspector: Set focus on the ExtensionPanel's iframe when it is selected
Summary: Web Inspector: Set focus on the ExtensionPanel's iframe when it is selected
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 97077 97084
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-07 14:16 PDT by johnjbarton
Modified: 2012-10-09 01:03 PDT (History)
14 users (show)

See Also:


Attachments
Patch (4.74 KB, patch)
2012-09-07 14:26 PDT, johnjbarton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description johnjbarton 2012-09-07 14:16:39 PDT
In WebInspector.Panel.wasShown() this.focus() is called, to set the focus within the panel the user just selected. For an ExtensionPanel, the focus should default to be within the iframe hosting the extension code, rather than its container (which, being a div, can't accept focus anyway).  By setting focus on the iframe, its contentWindow will become the window with focus.
Comment 1 johnjbarton 2012-09-07 14:26:42 PDT
Created attachment 162870 [details]
Patch
Comment 2 johnjbarton 2012-09-07 14:32:30 PDT
A different solution would store the extensionView on |this|, eg this._extensionView, then define ExtensionPanel.defaultFocusedElement() to return this._extensionView.defaultFocusedElement().  However the origin impl chose not to store the extensionView and perhaps there was a reason.
Comment 3 Andrey Kosyakov 2012-09-18 10:23:47 PDT
Comment on attachment 162870 [details]
Patch

LGTM. vsevik, could you please review this?
Comment 4 WebKit Review Bot 2012-09-18 11:02:15 PDT
Comment on attachment 162870 [details]
Patch

Clearing flags on attachment: 162870

Committed r128910: <http://trac.webkit.org/changeset/128910>
Comment 5 WebKit Review Bot 2012-09-18 11:02:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Csaba Osztrogonác 2012-09-18 23:37:09 PDT
(In reply to comment #4)
> (From update of attachment 162870 [details])
> Clearing flags on attachment: 162870
> 
> Committed r128910: <http://trac.webkit.org/changeset/128910>

The new inspector/extensions/extensions-panel.html fails on Qt and EFL:
--- /ramdisk/qt-linux-64-release/build/layout-test-results/inspector/extensions/extensions-panel-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/inspector/extensions/extensions-panel-actual.txt 
@@ -22,7 +22,7 @@
     show : <function>
 }
 Panel shown
-focused: true
+focused: false
 Extension panel size correct
 RUNNING TEST: extension_testSearch
 Panel hidden


Could you check it, please?
Comment 7 johnjbarton 2012-09-19 15:11:32 PDT
(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 162870 [details] [details])
> > Clearing flags on attachment: 162870
> > 
> > Committed r128910: <http://trac.webkit.org/changeset/128910>
> 
> The new inspector/extensions/extensions-panel.html fails on Qt and EFL:
> --- /ramdisk/qt-linux-64-release/build/layout-test-results/inspector/extensions/extensions-panel-expected.txt 
> +++ /ramdisk/qt-linux-64-release/build/layout-test-results/inspector/extensions/extensions-panel-actual.txt 
> @@ -22,7 +22,7 @@
>      show : <function>
>  }
>  Panel shown
> -focused: true
> +focused: false
>  Extension panel size correct
>  RUNNING TEST: extension_testSearch
>  Panel hidden
> 
> 
> Could you check it, please?

I'm uncertain who you are asking; if your are asking me I don't know how to check it.

Do these platforms have different focus behavior?
Comment 8 Csaba Osztrogonác 2012-10-09 01:03:48 PDT
I don't know anything about this test. But I'm sure it is a regression caused by http://trac.webkit.org/changeset/128910