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)

defect
Not set
normal

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.
Note that focusing on the address bar (ctrl+l) does make ctrl+page up/down work again.
According to https://plus.google.com/+SimonEugster/posts/Cr4iiZRN7Rd this happens on gmail too.
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
OS: Linux → All
Component: Tabbed Browser → Event Handling
Product: Firefox → Core
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)
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.
This is not actual regression. If keypress event is prevented its default, tab navigation never works.
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.
If the patch should be landed, I'll create automated tests.
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 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+
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)
(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.
(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)
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)
This fixes new orange because some keypress events are always consumed by tabs.
Attachment #8434096 - Flags: review?(bugs)
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+
(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 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 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+
Depends on: 1020832
QA Whiteboard: [good first verify]
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]
Blocks: 990512
No longer blocks: 990512
Depends on: 1052569
Depends on: 1067053
Depends on: 1068282
Depends on: 1096882
Depends on: 1200038
Blocks: 1137239
No longer blocks: 1137239
Depends on: 1137239
Blocks: 933470
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: