Closed
Bug 1008772
Opened 10 years ago
Closed 10 years ago
Web sites can steal ctrl+page up/down
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Core
DOM: UI Events & Focus Handling
Tracking
()
VERIFIED
FIXED
mozilla32
People
(Reporter: glandium, Assigned: masayuki)
References
(Depends on 2 open bugs, )
Details
(Keywords: regression)
Attachments
(3 files, 4 obsolete files)
15.30 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
10.58 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
9.79 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
STR: - Open several tabs - Open google plus (you need to be logged in) - Try switching tab with ctrl+page up/down. Expected result: - Tab switched Actual result: - Scrolling in google plus itself. This doesn't happen in ESR24. It looks like this started happening in 25.
Reporter | ||
Comment 1•10 years ago
|
||
Note that focusing on the address bar (ctrl+l) does make ctrl+page up/down work again.
Reporter | ||
Comment 2•10 years ago
|
||
According to https://plus.google.com/+SimonEugster/posts/Cr4iiZRN7Rd this happens on gmail too.
Reporter | ||
Comment 3•10 years ago
|
||
Related: https://support.mozilla.org/en-US/questions/977014
Updated•10 years ago
|
Keywords: regression,
regressionwindow-wanted
Comment 4•10 years ago
|
||
Regression window(m-i) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/12c11406ed75 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130724 Firefox/25.0 ID:20130724230740 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/949a5e125a8d Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130724 Firefox/25.0 ID:20130724230940 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=12c11406ed75&tochange=949a5e125a8d Regressed by:Bug 501496
Blocks: 501496
Keywords: regressionwindow-wanted
Updated•10 years ago
|
OS: Linux → All
Updated•10 years ago
|
Component: Tabbed Browser → Event Handling
Product: Firefox → Core
Assignee | ||
Comment 5•10 years ago
|
||
I'm not sure if this is actually a bug since shortcut keys which is implemented with <handler> element should be prevented its action by a call of preventDefault(). I guess that this is handled by here: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/tabbox.xml#141 I feel strange we allow only the shortcut keys which are handled in this method even if an active web page wants to prevent the browser's default behavior. Preventing these shortcut keys can cause some security issues? How do you think, smaug?
Assignee: nobody → masayuki
Status: NEW → UNCONFIRMED
Ever confirmed: false
Flags: needinfo?(bugs)
Assignee | ||
Comment 6•10 years ago
|
||
Changing Tabs and creating new tabs are not prevented by keydown.preventDefault() on both IE and Chrome. On IE, looks like almost of all shortcut keys work even if keydown event's preventDefault() is called. But on Chrome, looks like only tab navigation related shortcut keys work. However, e.g., Ctrl + S and Ctrl + F do not work.
Assignee | ||
Comment 7•10 years ago
|
||
This is not actual regression. If keypress event is prevented its default, tab navigation never works.
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 8•10 years ago
|
||
If tab navigation shouldn't blocked by contents, toolkit and browser should handle non-printable keys with keydown event handler in the system group rather than keypress event handler in the normal group.
Assignee | ||
Comment 9•10 years ago
|
||
If the patch should be landed, I'll create automated tests.
Assignee | ||
Comment 10•10 years ago
|
||
I'll request review to smaug since there is no reply from him...
Attachment #8424775 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8430564 -
Attachment is obsolete: true
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8430563 [details] [diff] [review] part.1 tabs should handle non-printable keys with keydown event handler in the system group rather than keypress event handler in the normal group Ignoring the defaultPrevented attribute and handling in the system group, keydown event is always handled by the tabbox. Even with this patch, some event which are handled by keypress event (i.e., printable keys) don't work since keypress event is suppressed by a call of preventDefault(). I think that we should handle them at keydown event if coming keydown event's default is already prevented. However, this approach should be done later because it needs to .key value which will be changed for latest D3E specs. If you think that the tab's keyboard navigation should be prevented by a call of preventDefault() of keydown event, I'll recreate the patch. Otherwise, I'll request additional review to enn.
Attachment #8430563 -
Flags: review?(bugs)
Flags: needinfo?(bugs)
Comment 14•10 years ago
|
||
Comment on attachment 8430563 [details] [diff] [review] part.1 tabs should handle non-printable keys with keydown event handler in the system group rather than keypress event handler in the normal group > >+ // Don't check if the event was already consumed because tab >+ // navigation should always work for security. Not really for security, but for better user experience, I think >+ > if (aEvent.ctrlKey && aEvent.shiftKey && !aEvent.metaKey) { >+ // XXX Should call peventDefault() only when the tab actually >+ // moved. preventDefault() >+ // Don't check if the event was already consumed because tab >+ // navigation should always work for security. Again, I'm not sure there is really much to do with security, but user experience. >+ > switch (event.keyCode) { > case event.DOM_VK_TAB: > if (event.ctrlKey && !event.altKey && !event.metaKey) >- if (this.tabs && this.handleCtrlTab) { >+ if (this.tabs && this.tabs.itemCount > 1 && this.handleCtrlTab) { > this.tabs.advanceSelectedTab(event.shiftKey ? -1 : 1, true); >- event.stopPropagation(); > event.preventDefault(); > } > break; > case event.DOM_VK_PAGE_UP: > if (event.ctrlKey && !event.shiftKey && !event.altKey && !event.metaKey) >- if (this.tabs && this.handleCtrlPageUpDown) { >+ if (this.tabs && this.tabs.itemCount > 1 && this.handleCtrlPageUpDown) { > this.tabs.advanceSelectedTab(-1, true); >- event.stopPropagation(); > event.preventDefault(); > } > break; > case event.DOM_VK_PAGE_DOWN: > if (event.ctrlKey && !event.shiftKey && !event.altKey && !event.metaKey) >- if (this.tabs && this.handleCtrlPageUpDown) { >+ if (this.tabs && this.tabs.itemCount > 1 && this.handleCtrlPageUpDown) { > this.tabs.advanceSelectedTab(1, true); >- event.stopPropagation(); > event.preventDefault(); > } > break; > case event.DOM_VK_LEFT: > if (event.metaKey && event.altKey && !event.shiftKey && !event.ctrlKey) >- if (this.tabs && this._handleMetaAltArrows) { >+ if (this.tabs && this.tabs.itemCount > 1 && this._handleMetaAltArrows) { > var offset = window.getComputedStyle(this, "") > .direction == "ltr" ? -1 : 1; > this.tabs.advanceSelectedTab(offset, true); >- event.stopPropagation(); > event.preventDefault(); > } > break; > case event.DOM_VK_RIGHT: > if (event.metaKey && event.altKey && !event.shiftKey && !event.ctrlKey) >- if (this.tabs && this._handleMetaAltArrows) { >+ if (this.tabs && this.tabs.itemCount > 1 && this._handleMetaAltArrows) { I don't understand "&& this.tabs.itemCount > 1" changes here. I think those additions should be removed. This needs a toolkit/browser peer review too. But I like the change, even though it is still unclear which keyboard commands should be overridable by web pages.
Attachment #8430563 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 15•10 years ago
|
||
Thank you, smaug. (In reply to Olli Pettay [:smaug] from comment #14) > Comment on attachment 8430563 [details] [diff] [review] > part.1 tabs should handle non-printable keys with keydown event handler in > the system group rather than keypress event handler in the normal group > I don't understand "&& this.tabs.itemCount > 1" changes here. > I think those additions should be removed. The event handler is changed for keydown from keypress. Therefore, I think that it's not caused default action, i.e., navigating tabs, following keypress event should be fired on content. How do you think about this? Should the tabs always consume tab navigation key events?
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(bugs)
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15) > Therefore, I think that it's not caused default action, i.e., navigating tabs, following > keypress event should be fired on content. I meant that if keydown events don't cause any tab navigation due to only one tab, we shouldn't consume the keydown event for firing following keypress event. However, it might make web developers misunderstand our behavior when they test with only one tab. If this inconsistency isn't good thing, we should consume the keydown event always, though.
Comment 17•10 years ago
|
||
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15) > The event handler is changed for keydown from keypress. Therefore, I think > that it's not caused default action, i.e., navigating tabs, following > keypress event should be fired on content. How do you think about this? > Should the tabs always consume tab navigation key events? I think for consistency consuming the key event should happen always.
Flags: needinfo?(bugs)
Assignee | ||
Comment 18•10 years ago
|
||
This patch makes tabs handle non-printable keys at keydown event handler in system group. By this change, tabs can handle events always 1. keypress events may not be fired if preceding keydown event is consumed. Therefore, tabs should handle keydown event as far as possible. 2. content may call stopPropagation() of keydown/keypress events. Therefore, tabs should handle events in system group. FYI: This still use keypress events since charCode is only set at keypress event. However, we will be able to use .key instead of charCode. Therefore, we may be able to fix it. However, it's not scope of this bug.
Attachment #8430563 -
Attachment is obsolete: true
Attachment #8433959 -
Flags: review?(enndeakin)
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8430730 -
Attachment is obsolete: true
Attachment #8433964 -
Flags: review?(enndeakin)
Assignee | ||
Comment 20•10 years ago
|
||
This fixes new orange because some keypress events are always consumed by tabs.
Attachment #8434096 -
Flags: review?(bugs)
Comment 21•10 years ago
|
||
Comment on attachment 8434096 [details] [diff] [review] part.3 Fix new orages caused by tabbrowser consuming some key events at keydown Do we run these tests on b2g or Android? The results might be different there. So make sure to push to try.
Attachment #8434096 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #21) > Comment on attachment 8434096 [details] [diff] [review] > part.3 Fix new orages caused by tabbrowser consuming some key events at > keydown > > Do we run these tests on b2g or Android? The results might be different > there. > So make sure to push to try. The editor tests are mochitest-chrome. Therefore, they don't run on mobile. The event count test is run only on Mac.
Comment 23•10 years ago
|
||
Comment on attachment 8433959 [details] [diff] [review] part.1 tabs should handle non-printable keys with keydown event handler in the system group rather than keypress event handler in the normal group (r=smaug) >- <handler event="keypress" keycode="VK_LEFT"> >+ <handler event="keydown" keycode="VK_LEFT" group="system"> > <![CDATA[ > var direction = window.getComputedStyle(this.parentNode, null).direction; > this.parentNode.advanceSelectedTab(direction == 'ltr' ? -1 : 1, this.arrowKeysShouldWrap); >+ // XXX Why don't you call preventDefault()? > ]]> I think we should add preventdefault="true" to these cases.
Attachment #8433959 -
Flags: review?(enndeakin) → review+
Comment 24•10 years ago
|
||
Comment on attachment 8433964 [details] [diff] [review] part.2 Add tests for tabbrowser key navigation which isn't blocked by content >+ // Kill the animation for simpler test. >+ Services.prefs.setBoolPref("browser.tabs.animate", false); >+ >+ if (gBrowser.mTabBox.handleCtrlTab) { I don't think handleCtrlTab is modified by default is it? We should run the test assuming the default true value. Same with handleCtrlPageUpDown. >+ let ltr = >+ window.getComputedStyle(gBrowser.mTabBox, "").direction == "ltr"; >+ let advanceKey = ltr ? "VK_RIGHT" : "VK_LEFT"; >+ let recessionKey = ltr ? "VK_LEFT" : "VK_RIGHT"; 'reverse' is a better word than 'recession'. Change this in the various places.
Attachment #8433964 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 25•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c142ccc5bb25 https://hg.mozilla.org/integration/mozilla-inbound/rev/721046203db0 https://hg.mozilla.org/integration/mozilla-inbound/rev/6d8836257099
Comment 26•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c142ccc5bb25 https://hg.mozilla.org/mozilla-central/rev/721046203db0 https://hg.mozilla.org/mozilla-central/rev/6d8836257099
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•10 years ago
|
QA Whiteboard: [good first verify]
Comment 27•10 years ago
|
||
Reproduced the issue on Nightly (2014-05-28), verified as fixed on latest Aurora 32.0a2 (buildID: 20140710004005) under Mac OS X 10.9, Ubuntu 64bit and Windows 7 64bit.
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify] → [good first verify] [testday-20140711]
Updated•5 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•