Closed Bug 1268444 Opened 4 years ago Closed 3 years ago

Convert network monitor to use new key shortcut API

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox52 verified)

VERIFIED FIXED
Firefox 52
Iteration:
52.3 - Nov 14
Tracking Status
firefox52 --- verified

People

(Reporter: ochameau, Assigned: gasolin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(1 file)

We need to stop using xul for key shortcuts and instead use the upcoming module from bug 1260493.
http://mxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/netmonitor.xul#85
Whiteboard: [devtools-html][triage]
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [devtools-html][triage] → [devtools-html]
Priority: P2 → P3
Whiteboard: [devtools-html] → [reserve-html]
Hi Alex, will this bug require QA verification?
Flags: needinfo?(poirot.alex)
Yes. We would need to verify that related key shortcuts works the same.
Flags: needinfo?(poirot.alex)
Flags: qe-verify? → qe-verify+
QA Contact: petruta.rasa
Whiteboard: [reserve-html] → [reserve-html][devtools-html]
Whiteboard: [reserve-html][devtools-html] → [reserve-html]
No longer blocks: devtools-html-1
Priority: P3 → --
Whiteboard: [reserve-html]
QA Contact: petruta.rasa → ciprian.georgiu
Whiteboard: [netmonitor]
Priority: -- → P2
Depends on: 1308500
What needs to be done:

1. convert key freeTextFilterKey&command in netmonitor.xul
2. convert requestsMenuSortKeyboardEvent in reqeust-menu-view
3. convert requestsMenuFilterKeyboardEvent in reqeust-menu-view

4. remove netmonitor.dtd if its land after bug 1308440
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Priority: P2 → P1
Iteration: --- → 52.3 - Nov 7
Instead of Comment 3, at the end I found only freeTextFilterKey&command in netmonitor.xul is needed to migrate
Note: I refered `inspector.js` to put shortcut binding in panel.js
https://github.com/mozilla/gecko-dev/blob/master/devtools/client/inspector/inspector.js#L379
Comment on attachment 8802051 [details]
Bug 1268444 - Convert network monitor to use new key shortcut API;

https://reviewboard.mozilla.org/r/86604/#review85564

Thanks for working on this!

See my inline comments.

Honza

::: devtools/client/locales/en-US/netmonitor.properties:482
(Diff revision 1)
>  netmonitor.footer.filterOther=Other
>  
>  # LOCALIZATION NOTE (netmonitor.footer.filterFreetext): This is the label displayed
>  # in the network details footer for the url filtering textbox.
>  netmonitor.footer.filterFreetext.label=Filter URLs
> -netmonitor.footer.filterFreetext.key=F
> +netmonitor.footer.filterFreetext.key=CmdOrCtrl+F

If you are changing a string you also need to change the string ID so, localizers are notified about the change. See also: https://bugzilla.mozilla.org/show_bug.cgi?id=1309191

::: devtools/client/netmonitor/panel.js:34
(Diff revision 1)
> +  });
> +  shortcuts.on(L10N.getStr("netmonitor.footer.filterFreetext.key"), (name, event) => {
> +    event.preventDefault();
> +    this._view.RequestsMenu.freetextFilterBox.focus();
> +  });
>  }

Shouldn't you call shortcuts.destroy() to clean up the 'keydown' listener?
Attachment #8802051 - Flags: review?(odvarko)
Issue addressed, please kindly review it
Note: `netmonitor.footer.filterFreetext.key` is renamed to `netmonitor.toolbar.filterFreetext.key` to align with renaming in Bug 1309191
Comment on attachment 8802051 [details]
Bug 1268444 - Convert network monitor to use new key shortcut API;

https://reviewboard.mozilla.org/r/86604/#review85832

LGTM!

Honza
Attachment #8802051 - Flags: review?(odvarko) → review+
Comment on attachment 8802051 [details]
Bug 1268444 - Convert network monitor to use new key shortcut API;

https://reviewboard.mozilla.org/r/86604/#review85836

R+ assuming Try is green

Honza
The try is green (with 1 unrelated framework/registration error)

thanks!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1f05cfb4a8dc
Convert network monitor to use new key shortcut API;r=Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1f05cfb4a8dc
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
This issue is verified fixed on latest Nightly build 52.0a1 from 2016-10-20 under the following OSes:
- Windows 10 x64
- Ubuntu 16.04 x64
- macOS 10.12.1 Beta

The related network monitor shortcut key (Ctrl/Cmd+F) is still working the same on all platforms. Marking here accordingly.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.