1488055 - It isn't possible to open a link from the sidebar in a new tab

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P2)

Description

[fx4waldi]

Attachment: Bug Sidebar.xpi

STR:
1. Install the extension from the attachment.
2. Open the sidebar and try to open the link in a new tab.

Clicking the middle mouse button and click while holding down the Control key doesn't work. The link can be opened in a new tab only by using the context menu (Open Link in New Tab).

The problem also occurs in the extension "Side View" https://github.com/mozilla/side-view/issues/317

I used the mozregression-gui tool, results below.

Last good build:
build_date: 2018-08-14 17:04:31.422000
changeset: 78d5bc33afd08d7fa8acedcdf074de4ddc6f0bf0
repo_name: mozilla-inbound

First bad build:

app_name: firefox
build_date: 2018-08-14 17:12:58.726000
changeset: 8920abb9801dcf5a330485b7393dce3265487660
repo_name: mozilla-inbound

pushlog_url: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=78d5bc33afd08d7fa8acedcdf074de4ddc6f0bf0&tochange=8920abb9801dcf5a330485b7393dce3265487660

Comment 1

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Regression in 63 in common browser UI.

Comment 2

[Shane Caraveo (:mixedpuppy)]

It wasn't caused by the regression pointed out.  IIRC sideview turned off browser_style at some point.  A bug introduced in bug 1330369 causes one of our content scripts to only load when browser_style is true, and that content script is what sets isAppTab.  That setting is what forces links to open into new tabs.  All our other content panels are handled different, so only the sidebar is affected.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1488055 fix loading ext-browser-content script in sidebar when browser_style=false, r?rpl

Comment 4

[Shane Caraveo (:mixedpuppy)]

Wil, can someone verify the patch with side view?  I verified with the attached extension.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 9007039 [details]
Bug 1488055 fix loading ext-browser-content script in sidebar when browser_style=false, r?rpl

Luca Greco [:rpl] has approved the revision.

Comment 6

[Wil Clouser [:clouserw]]

Ian is working on some analytics in Side View these days.  Ian - are you able to test this also?  Thanks

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 9007039 [details]
Bug 1488055 fix loading ext-browser-content script in sidebar when browser_style=false, r?rpl

Luca Greco [:rpl] has requested changes to the revision.

Comment 8

[Ian Bicking (:ianbicking)]

I haven't built Firefox on my current laptop yet, and my build setup is causing me some problems. But Side View is installable via https://testpilot.firefox.com/experiments/side-view – can you verify with that?

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 9007039 [details]
Bug 1488055 fix loading ext-browser-content script in sidebar when browser_style=false, r?rpl

Luca Greco [:rpl] has approved the revision.

Comment 10

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e93a327c248957877f0fb7139da39e92d0c74cc

Comment 11

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9736edbe0b868ed75d12697f4445653ed5b4f7a6

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/353cb0f90dc4
fix loading ext-browser-content script in sidebar when browser_style=false, r=rpl

Comment 13

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/353cb0f90dc4

Comment 14

[Pascal Chevrel:pascalc]

Shane, could your request an uplift to beta when you get a chance? Thanks

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9007039 [details]
Bug 1488055 fix loading ext-browser-content script in sidebar when browser_style=false, r?rpl

Approval Request Comment
[Feature/Bug causing the regression]:1330369
[User impact if declined]: external domain clicks in sidebars do not open in tabs as they should
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: see comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: change is minimal, limited to extension sidebar
[String changes made/needed]: none

Comment 16

[Pascal Chevrel:pascalc]

Comment on attachment 9007039 [details]
Bug 1488055 fix loading ext-browser-content script in sidebar when browser_style=false, r?rpl

Fix for an identified web extensions regression in primary UI, uplift approved for 63 beta 6. Thanks

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/47094cecc7e1

Comment 18

[CosminB]

I noticed different behaviors between the good, affected and fixed build.

Behaviors:
FF 63.0a1(20180813100104) - good
Click inside the extension sidebar: opens the link inside the extension sidebar.
Ctrl+left click inside the extension sidebar: opens a new tab with that link.
Ctrl+middle mouse button inside the extension sidebar: opens a new tab with that link.

FF 64.0a1(20180906100252) - affected
Click inside the extension sidebar: opens the link inside the extension sidebar.
Ctrl+left click inside the extension sidebar: does nothing.
Ctrl+middle mouse button inside the extension sidebar: does nothing.

FF 64.0a1(20180913100107) - fixed
Click inside the extension sidebar: opens a new tab with that link.
Ctrl+left click inside the extension sidebar: does nothing.
Ctrl+middle mouse button inside the extension sidebar: does nothing.

Could you please let me know if these are the intended changes?

Thanks!

Comment 19

[CosminB]

The OS where I reproduced these scenarios was Win 7 64-bit with the extension from the Description.

Comment 20

[Shane Caraveo (:mixedpuppy)]

(In reply to CosminB from comment #18)
> I noticed different behaviors between the good, affected and fixed build.
> 
> Behaviors:
> FF 63.0a1(20180813100104) - good

The behavior was not correct in that to start with.

First, the browser_style setting changed the behavior here, which it should not have.  That is the primary fix in this patch.

The intended behavior would match tabs that are pinned.  That specifically is, a link to a non-same-origin domain should (by default) open in a tab unless it makes changes to the target attribute.

a href="foo.com" 
  click should open into new a active tab
  cmd+click should open into a new background tab
  
a href="/sameorigin.html" 
  click should replace the page in the sidebar
  cmd+click should open into a new background tab

I do see that this fix only addressed the default click, so I'll need to follow up with another fix.  ni? felipe to see if he knows the actor changes enough to suggest a fix.

I'll also attach a new extension with a more comprehensive STR

Comment 21

[Shane Caraveo (:mixedpuppy)]

Attachment: sidebar.xpi

str:

- load extension
- click "bugzilla" 
  - expect bugzilla in new active tab
- click "another sidebar"
  - expect sidebar page to change (note the link text changed)
- cmd+click "bugzilla"
  - expect bugzilla in new background tab
- cmd+click sidebar link
  - expect sidebar in new tab
- context menu-open in tab for the sidebar link
- move extension tab to a pinned tab
- retest above from pinned tab to see actual expected behaviors
  - sidebar should behave the same as the pinned tab

Comment 22

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1488055 - followup fix to enable the click handler actor in sidebar, r?mconley

Depends on D5196

Comment 23

[Shane Caraveo (:mixedpuppy)]

mconley helped me figure out the fix for the other click handling, followup has been attached.  this will require manual verification with QA per the STR in comment 21

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 9008793 [details]
Bug 1488055 - followup fix to enable the click handler actor in sidebar, r?mconley

Mike Conley (:mconley) (:⚙️) has approved the revision.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4476938f7d7
followup fix to enable the click handler actor in sidebar, r=mconley

Comment 26

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9008793 [details]
Bug 1488055 - followup fix to enable the click handler actor in sidebar, r?mconley

Approval Request Comment
[Feature/Bug causing the regression]: 1472491
[User impact if declined]: modified clicks (e.g. ctrl/cmd+click) dont work
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet, making request early
[Needs manual test from QE? If yes, steps to reproduce]: yes, see comment 21
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: This re-enables click handling that changed in 1472491 and was omitted from extension panels and other non-tab browsers.
[String changes made/needed]: none

Comment 27

[Stephan Sokolow]

A note for anyone evaluating the approval:

I don't know whether it's a bug or feature, but the about:config settings I use to prevent sites from being able to open new tabs or windows without my middle-clicking the links in question also cause left-click to navigate pinned tabs.

I rely on middle-click to open new tabs from pinned tabs so, if the current fix code matches pinned tabs but without enabling middle-click, I'm stuck with the buggy behaviour of having to always choose "in new tab" from the context menu.

Comment 28

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/42174b72eda5
Backed out changeset f4476938f7d7 for browser chrome failures on browser_address_edit. CLOSED TREE

Comment 29

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed,busted,exception&revision=f4476938f7d753576e8eb481d6865859d6090665&selectedJob=199140761&searchStr=os,x,10.10,opt,mochitests,with,e10s,test-macosx64%2Fopt-mochitest-browser-chrome-e10s-4,m-e10s(bc4)

Failure log:https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed,busted,exception&revision=f4476938f7d753576e8eb481d6865859d6090665&selectedJob=199140761&searchStr=os,x,10.10,opt,mochitests,with,e10s,test-macosx64%2Fopt-mochitest-browser-chrome-e10s-4,m-e10s(bc4)

 11:17:05     INFO - showPayment@jar:file:///Users/cltbld/tasks/task_1536862005/build/application/Firefox%20Nightly.app/Contents/Resources/browser/omni.ja!/components/paymentUIService.js:63:5
11:17:05     INFO - 
11:17:05     INFO - Buffered messages finished
11:17:05     INFO - TEST-UNEXPECTED-FAIL | browser/components/payments/test/browser/browser_address_edit.js | [JavaScript Error: "window.whereToOpenLink is not a function" {file: "resource:///modules/ContentClick.jsm" line: 57}]
11:17:05     INFO - contentAreaClick@resource:///modules/ContentClick.jsm:57:17
11:17:05     INFO - receiveMessage@resource:///modules/ContentClick.jsm:22:9
11:17:05     INFO - receiveMessage@jar:file:///Users/cltbld/tasks/task_1536862005/build/application/Firefox%20Nightly.app/Contents/Resources/browser/omni.ja!/components/nsBrowserGlue.js:549:30
11:17:05     INFO - showPayment@jar:file:///Users/cltbld/tasks/task_1536862005/build/application/Firefox%20Nightly.app/Contents/Resources/browser/omni.ja!/components/paymentUIService.js:63:5
11:17:05     INFO -  - 
11:17:05     INFO - Stack trace:
11:17:05     INFO - chrome://mochitests/content/browser/browser/components/payments/test/browser/head.js:onConsoleMessage:351
11:17:05     INFO - chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:observe/<:387
11:17:05     INFO - jar:file:///Users/cltbld/tasks/task_1536862005/build/application/Firefox%20Nightly.app/Contents/Resources/browser/omni.ja!/components/paymentUIService.js:showPayment:63
11:17:05     INFO - Console message: [JavaScript Error: "Content Security Policy: The page’s settings blocked the loading of a resource at inline (“default-src”)." {file: "resource://payments/paymentRequest.xhtml" line: 0}]
11:17:05     INFO - manuallyAddShippingAddress, fill in address form with options: {"expectPersist":true,"isEditing":false,"setPersistCheckedValue":true,"checkboxSelector":"#address-page .persist-checkbox"}

Comment 30

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=388419e42164841acfa2e42191e4f22d0399d001

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00fa9c00facf
followup fix to enable the click handler actor in sidebar, r=mconley

Comment 32

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/00fa9c00facf

Comment 33

[Pascal Chevrel:pascalc]

Comment on attachment 9008793 [details]
Bug 1488055 - followup fix to enable the click handler actor in sidebar, r?mconley

Approved for 63 beta 7, thanks.

Comment 34

[Shane Caraveo (:mixedpuppy)]

Not sure this is worth a release note, but if it were, I'd say something akin to: fixed: link clicks within an extension sidebar behaved differently based on the value of the sidebar browser_style.

Comment 35

[CosminB]

Attachment: Bug1488055.gif

Thanks Shane!

This issue is verified as fixed on Firefox 64.0a1(20180916220033) under Win 7 64-bit and Mac OS X 10.13.3.

Please see the attached video.

Comment 36

[CosminB]

Attachment: Bug1488055Beta.gif

I tested on Firefox 63.0b7(20180917143811) under Win 7 64-bit and Mac OS X 10.13.3 and noticed that these 2 scenarios are not working:
- cmd+click "bugzilla"
  - expect bugzilla in new background tab
- cmd+click sidebar link
  - expect sidebar in new tab

The rest of the scenarios pass the tests.

Comment 37

[CosminB]

The scenarios from above are not working in the normal or pinned tabs.

Comment 38

[Shane Caraveo (:mixedpuppy)]

Probably because the second patch has not landed on b7 yet.

Comment 39

[Pascal Chevrel:pascalc]

I think the second uplift in comment #26 was missed for beta 7 because the status for Firefox 63 had been marked as 'fixed' with the first uplift in comment #17 and wasn't reset to 'affected' later.

I am resetting it to 'affected' so as it gets back on sheriffs radar and gets included in Beta 8.

Comment 40

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c271ac97a512

Comment 41

[CosminB]

Retested all the scenarios on 63.0b8(20180920135444) under Win 7 64-bit and Mac OS X 10.13.3 and everything looks good.