Web sites can steal ctrl+page up/down

VERIFIED FIXED in mozilla32

Status

()

Core
Event Handling
VERIFIED FIXED
3 years ago
9 months ago

People

(Reporter: glandium, Assigned: masayuki)

Tracking

(Depends on: 5 bugs, Blocks: 2 bugs, {regression})

unspecified
mozilla32
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 4 obsolete attachments)

15.30 KB, patch
Neil Deakin (not available until Aug 9)
: review+
Details | Diff | Splinter Review
10.58 KB, patch
Neil Deakin (not available until Aug 9)
: review+
Details | Diff | Splinter Review
9.79 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
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

3 years ago
Note that focusing on the address bar (ctrl+l) does make ctrl+page up/down work again.
(Reporter)

Comment 2

3 years ago
According to https://plus.google.com/+SimonEugster/posts/Cr4iiZRN7Rd this happens on gmail too.
(Reporter)

Comment 3

3 years ago
Related: https://support.mozilla.org/en-US/questions/977014

Updated

3 years ago
Keywords: regression, regressionwindow-wanted

Comment 4

3 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

3 years ago
OS: Linux → All

Updated

3 years ago
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.
Blocks: 968056
This is not actual regression. If keypress event is prevented its default, tab navigation never works.
Created attachment 8424775 [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

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.
Created 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'll request review to smaug since there is no reply from him...
Attachment #8424775 - Attachment is obsolete: true
Created attachment 8430564 [details] [diff] [review]
part.2 Add tests for tabbrowser key navigation which isn't blocked by content
Created attachment 8430730 [details] [diff] [review]
part.2 Add tests for tabbrowser key navigation which isn't blocked by content
Attachment #8430564 - Attachment is obsolete: true
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)
Created 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)

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)
Created attachment 8433964 [details] [diff] [review]
part.2 Add tests for tabbrowser key navigation which isn't blocked by content
Attachment #8430730 - Attachment is obsolete: true
Attachment #8433964 - Flags: review?(enndeakin)
Created attachment 8434096 [details] [diff] [review]
part.3 Fix new orages caused by tabbrowser consuming some key events at keydown

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+
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
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
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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]

Updated

3 years ago
Blocks: 990512

Updated

3 years ago
No longer blocks: 990512
Duplicate of this bug: 990512

Updated

3 years ago
Depends on: 1052569

Updated

3 years ago
Depends on: 1067053

Updated

3 years ago
Depends on: 1068282

Updated

3 years ago
Depends on: 1096882

Updated

2 years ago
Depends on: 1200038

Updated

2 years ago
Blocks: 1137239

Updated

2 years ago
No longer blocks: 1137239
Depends on: 1137239

Updated

9 months ago
Blocks: 933470
You need to log in before you can comment on or make changes to this bug.