Bug 96148

Summary: Web Inspector: Set focus on the ExtensionPanel's iframe when it is selected
Product: WebKit Reporter: johnjbarton <johnjbarton>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, caseq, cdumez, joepeck, keishi, loislo, ossy, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 97077, 97084    
Bug Blocks:    
Attachments:
Description Flags
Patch none

johnjbarton
Reported 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.
Attachments
Patch (4.74 KB, patch)
2012-09-07 14:26 PDT, johnjbarton 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
johnjbarton
Comment 1 2012-09-07 14:26:42 PDT
Created attachment 162870 [details] Patch
johnjbarton
Comment 2 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.
Andrey Kosyakov
Comment 3 2012-09-18 10:23:47 PDT
Comment on attachment 162870 [details] Patch LGTM. vsevik, could you please review this?
WebKit Review Bot
Comment 4 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>
WebKit Review Bot
Comment 5 2012-09-18 11:02:18 PDT
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 6 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?
johnjbarton
Comment 7 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?
Csaba Osztrogonác
Comment 8 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
Note You need to log in before you can comment on or make changes to this bug.