Closed Bug 1230325 Opened 9 years ago Closed 9 years ago

Ctrl+S doesn't save the page if a node in markup-view is focused. It scrolls the element into view instead

Categories

(DevTools :: Inspector, defect, P1)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox45 affected, firefox46 fixed, firefox47 fixed, firefox48 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 48
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: arni2033, Assigned: jdescottes)

References

Details

(Keywords: regression, Whiteboard: [btpp-fix-now])

Attachments

(2 files, 3 obsolete files)

MozReview Request: Bug 1230325 - markup-view: skip keyboard shortcuts if any modifier;r=pbrosset
58 bytes, text/x-review-board-request
pbro
: review+
ritu
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta+
Details
bug1230325.rebased-beta.patch
8.48 KB, patch
Details | Diff | Splinter Review
>>> My Info: Win7_64, Nightly 45, 32bit, ID 20151201030226 STR: 1. Open Inspector on this page 2. Click on <head> in markup-view 3. Press Ctrl+S to save the page Result: No visible action Expectations: Page should be saved. Note: Additionally, if user presses Shift+S / Ctrl+Shift+S / Alt+S (e.g. custom hotkey to save page w/o prompt), the event should propagate normally to allow those custom hotkeys to work OK. Also, all other key pressings should propagate if they aren't documented shortcuts. I'm going to file bugs for fixing other shortcuts if nobody stops me. Regression range: > https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e5bd632ebaff750362caf120a3796f9ad48106aa&tochange=0d7793440e17f415ef4937db7fbf0abc679d4f66
Oh, well, even if the plan was to prevent event propagation from devtools, Ctrl+S still shouldn't scroll element into view, because there's only 1 shortcut for that - simply "S". The same for other shortcuts (e.g. Alt+F2 / Ctrl+F2 shouldn't start editing HTML of a node)
Summary: Ctrl+S doesn't save the page if a node in markup-view is focused → Ctrl+S doesn't save the page if a node in markup-view is focused. It scrolls the element into view instead
See Also: → 1193248
Has STR: --- → yes
Priority: -- → P1
Whiteboard: [btpp-fix-now]
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Attached patch bug1230325.wip.patch (obsolete) — — Splinter Review
pbro: Can you give me your feedback on: - skipping modifiers for ALL shortcuts of markup view - modifying the test actor to enable the new test (see commit message below) ==================================================================== Bug 1230325 - markup-view: skip keyboard shortcuts if any modifier;r=pbro Bail out from the markup view onKeyDown handler if any modifier is currently true. All shortcuts specified in this handler are intended to be used without modifier, so for now this approach is fine. If we decide to support some keys with modifiers, this will need to change. Added a test checking the use case mentioned in Bug 1230325, with the S shortcut. In order to write the test, had to create an additional method on the test actor to be able to wait for events in the window of the content process.
Attachment #8726813 - Flags: feedback?(pbrosset)
Attached patch bug1230325.wip.patch (obsolete) — — Splinter Review
Typo in the previous patch. Try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=44aeff9e0af1
Attachment #8726813 - Attachment is obsolete: true
Attachment #8726813 - Flags: feedback?(pbrosset)
Attachment #8726965 - Flags: feedback?(pbrosset)
e10s failures in the previous try push! new try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=e724ef246653
Attachment #8726965 - Attachment is obsolete: true
Attachment #8726965 - Flags: feedback?(pbrosset)
Attachment #8727058 - Flags: feedback?(pbrosset)
Comment on attachment 8727058 [details] [diff] [review] bug1230325.wip.patch (fixed e10s failures) Review of attachment 8727058 [details] [diff] [review]: ----------------------------------------------------------------- I like the new test! Thanks. The changes to test-actor.js look good to me, this is a useful method to add to this actor. I just made a comment regarding its name and signature. As to bailing out of the keyDown handler, I think this works fine. As you said, none of the keys require a modifier right now. ::: devtools/client/inspector/inspector-panel.js @@ +1230,5 @@ > > /** > * Scroll the node into view. > */ > + scrollNodeIntoView: function(evt) { Why add `evt` here? ::: devtools/client/inspector/markup/markup.js @@ +604,5 @@ > if (this._isInputOrTextarea(event.target)) { > return; > } > > + // Ignore keystrokes with modifiers Maybe worth explaining that this is important to let browser shortcuts (like save) bubble through? ::: devtools/client/inspector/markup/test/browser_markup_keybindings_scrolltonode.js @@ +61,5 @@ > + yield checkElementIsInViewport("#scroll-bottom", false, testActor); > +}); > + > +/** > + * Verify that the element matching the provided selected is either in or out s/selected/selector ::: devtools/client/shared/test/test-actor.js @@ +231,5 @@ > + * @param {String} eventName The name of the event to listen to > + * @param {String} selector The css selector of the node which should trigger > + * the event. Pass the empty string to target the window. > + */ > + waitForSelectorEvent: protocol.method(function (eventName, selector) { Not sure about the word selector in the method name, maybe "waitForEventOnNode" ? @@ +242,5 @@ > + }); > + }, { > + request: { > + eventName: Arg(0, "string"), > + selector: Arg(1, "string") To avoid having to pass an empty string, you can decorate this type like this: selector: Arg(1, "nullable:string")
Attachment #8727058 - Flags: feedback?(pbrosset) → feedback+
Bail out from the markup view onKeyDown handler if any modifier is currently true. All shortcuts specified in this handler are intended to be used without modifier, so for now this approach is fine. Added a test checking the use case mentioned in Bug 1230325, with the S shortcut. In order to write the test, had to create an additional method on the test actor to be able to wait for events in the window of the content process. Review commit: https://reviewboard.mozilla.org/r/38447/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38447/
Attachment #8727327 - Flags: review?(pbrosset)
Thanks for the feedback ! (In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #6) > Comment on attachment 8727058 [details] [diff] [review] > ::: devtools/client/inspector/inspector-panel.js > @@ +1230,5 @@ > > > > /** > > * Scroll the node into view. > > */ > > + scrollNodeIntoView: function(evt) { > > Why add `evt` here? > Leftover! Removed. > > ::: devtools/client/shared/test/test-actor.js > @@ +231,5 @@ > > + * @param {String} eventName The name of the event to listen to > > + * @param {String} selector The css selector of the node which should trigger > > + * the event. Pass the empty string to target the window. > > + */ > > + waitForSelectorEvent: protocol.method(function (eventName, selector) { > > Not sure about the word selector in the method name, maybe > "waitForEventOnNode" ? > Thanks! I really didn't like the previous name either, but couldn't figure out a good one. This one is perfect. > @@ +242,5 @@ > > + }); > > + }, { > > + request: { > > + eventName: Arg(0, "string"), > > + selector: Arg(1, "string") > > To avoid having to pass an empty string, you can decorate this type like > this: > selector: Arg(1, "nullable:string") Ah thanks, could not find a way to have optional arguments from my limited investigations. Should have asked.
Comment on attachment 8727327 [details] MozReview Request: Bug 1230325 - markup-view: skip keyboard shortcuts if any modifier;r=pbrosset https://reviewboard.mozilla.org/r/38447/#review35007
Attachment #8727327 - Flags: review?(pbrosset) → review+
https://hg.mozilla.org/mozilla-central/rev/5a8c334ac296
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Attachment #8727058 - Attachment is obsolete: true
Comment on attachment 8727327 [details] MozReview Request: Bug 1230325 - markup-view: skip keyboard shortcuts if any modifier;r=pbrosset Approval Request Comment [Feature/regressing bug #]:Fix regression from Bug 1203147 [User impact if declined]: User can't save using the keyboard shortcut (ctrl+S) when devtools inspector is focused. [Describe test coverage new/current, TreeHerder]: Added new test covering both the feature in Bug 1203147 and this bug. Try : https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=5a8c334ac296 [Risks and why]: Low risk. Small code change: simply skipping keyboard events if they have any modifier. Most of the changeset is dealing with test code. [String/UUID change made/needed]:none
Attachment #8727327 - Flags: approval-mozilla-beta?
Attachment #8727327 - Flags: approval-mozilla-aurora?
Comment on attachment 8727327 [details] MozReview Request: Bug 1230325 - markup-view: skip keyboard shortcuts if any modifier;r=pbrosset regression, taking it.
Attachment #8727327 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8727327 [details] MozReview Request: Bug 1230325 - markup-view: skip keyboard shortcuts if any modifier;r=pbrosset Test fixes along with a fix for keyboard shortcuts. This should land for beta 2.
Attachment #8727327 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I'm hitting conflicts uplifting this to beta: grafting 332443:ad5cb3578e98 "Bug 1230325 - markup-view: skip keyboard shortcuts if any modifier;r=pbrosset a=ritu" merging devtools/client/inspector/markup/markup.js merging devtools/client/inspector/markup/test/browser.ini merging devtools/client/shared/test/test-actor.js warning: conflicts while merging devtools/client/inspector/markup/markup.js! (edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue Can we get a rebased patch?
Flags: needinfo?(jdescottes)
Here is a rebased version of the patch on top of beta. Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=c398e354f8ff
Flags: needinfo?(jdescottes)
[bugday-20160323] Status: RESOLVED,FIXED -> VERIFIED Comments: Test successful Component: Name Firefox Version 46.0b9 Build ID 20160322075646 Update Channel beta User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 OS Windows 7 SP1 x86_64 Expected Results: Page has saved Actual Results: As expected
I have successfully reproduce this bug on firefox nightly 45.0a1 (2015-12-03) with windows 7 (32 bit) Mozilla/5.0 (Windows NT 6.1; rv:45.0) Gecko/20100101 Firefox/45.0 I found this fix on latest beta 46.0b9 Mozilla/5.0 (Windows NT 6.1; rv:46.0) Gecko/20100101 Firefox/46.0 Build ID : 20160407053945 I also found this fix on latest developer edition 47.0a2 (2016-04-19) Mozilla/5.0 (Windows NT 6.1; rv:47.0) Gecko/20100101 Firefox/47.0 Build ID : 20160419004026 And I also found this fix on latest nightly 48.0a1 (2016-04-19 Mozilla/5.0 (Windows NT 6.1; rv:48.0) Gecko/20100101 Firefox/48.0 Build ID : 20160419030312
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: