Skip to content
This repository has been archived by the owner on Feb 29, 2020. It is now read-only.

fix(topstories): Ctrl-click/middle mouse button does not open new tab #3046

Merged
merged 1 commit into from
Jul 31, 2017

Conversation

csadilek
Copy link
Collaborator

Fixes #3033.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.407% when pulling f66248d on csadilek:fix-3033 into c260e0a on mozilla:master.

this.props.dispatch(ac.SendToMain({type: at.OPEN_LINK, data: this.props.link}));
this.props.dispatch(ac.SendToMain({
type: at.OPEN_LINK,
data: Object.assign(this.props.link, {where: (event.ctrlKey || event.metaKey || event.button === 1) ? "tab" : "current"})
Copy link
Member

@Mardak Mardak Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this supports "tab" but not the other click behaviors, e.g., "window" (on shift) and "save" (on alt) or other behaviors supported in whereToOpenLink:
https://searchfox.org/mozilla-central/source/browser/base/content/utilityOverlay.js#111-159

Not sure if there's a good way to just send the event over to call that method on the browser.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I didn't find one :(. We have support for "window" though via the context menu (not shortcut) (incl. private windows).

I think this is better than not having it for 56, but I will leave it up to you guys. I am fine either way and can dig in more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you could callbrowser.ownerGlobal.whereToOpenLink with the event, if you can pass the whole event up

Copy link
Member

@Mardak Mardak Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A fake event should work to get across the message boundary:

const {altKey, button, ctrlKey, metaKey, shiftKey} = event;
dispatch({
  data:  {event: {altKey, button, ctrlKey, metaKey, shiftKey},}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mardak ah I will try that. Was just looking for ways to get the event across again.

Copy link
Contributor

@k88hudson k88hudson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can get this to work with whereToOpenLink that seems like it would be ideal. Otherwise, this is probably good enough for 56

this.props.dispatch(ac.SendToMain({type: at.OPEN_LINK, data: this.props.link}));
this.props.dispatch(ac.SendToMain({
type: at.OPEN_LINK,
data: Object.assign(this.props.link, {where: (event.ctrlKey || event.metaKey || event.button === 1) ? "tab" : "current"})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you could callbrowser.ownerGlobal.whereToOpenLink with the event, if you can pass the whole event up

@k88hudson k88hudson assigned csadilek and unassigned k88hudson Jul 31, 2017
@csadilek
Copy link
Collaborator Author

OK, I can give it another try.

@csadilek
Copy link
Collaborator Author

@Mardak @k88hudson thanks, updated.

@csadilek csadilek assigned k88hudson and unassigned csadilek Jul 31, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.407% when pulling c49dfde on csadilek:fix-3033 into c260e0a on mozilla:master.

Copy link
Member

@Mardak Mardak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice. I just tested with shift-cmd-click!

@Mardak Mardak merged commit 2445903 into mozilla:master Jul 31, 2017
@as-pine-proxy
Copy link
Collaborator

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants