Closed Bug 1230325 Opened 4 years ago Closed 4 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)

defect

Tracking

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

RESOLVED FIXED
Firefox 48
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)

>>>   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: 4 years ago
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.