Closed Bug 1488055 Opened 1 year ago Closed 1 year ago

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

Categories

(WebExtensions :: General, defect, P2)

defect

Tracking

(firefox-esr60 unaffected, firefox61 unaffected, firefox62 unaffected, firefox63+ verified, firefox64 verified)

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- unaffected
firefox63 + verified
firefox64 --- verified

People

(Reporter: fx4waldi, Assigned: mixedpuppy)

References

Details

(Keywords: regression)

Attachments

(6 files)

Attached file 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
Blocks: 1472491
Flags: needinfo?(kmaglione+bmo)
Keywords: regression
[Tracking Requested - why for this release]:
Regression in 63 in common browser UI.
Flags: needinfo?(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.
Assignee: nobody → mixedpuppy
Blocks: 1330369
No longer blocks: 1472491
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mixedpuppy)
Priority: -- → P2
Flags: needinfo?(kmaglione+bmo)
Wil, can someone verify the patch with side view?  I verified with the attached extension.
Flags: needinfo?(wclouser)
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.
Attachment #9007039 - Flags: review+
Ian is working on some analytics in Side View these days.  Ian - are you able to test this also?  Thanks
Flags: needinfo?(wclouser) → needinfo?(ianb)
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.
Attachment #9007039 - Flags: review+
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?
Flags: needinfo?(ianb)
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.
Attachment #9007039 - Flags: review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/353cb0f90dc4
fix loading ext-browser-content script in sidebar when browser_style=false, r=rpl
https://hg.mozilla.org/mozilla-central/rev/353cb0f90dc4
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Shane, could your request an uplift to beta when you get a chance? Thanks
Flags: needinfo?(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
Flags: needinfo?(mixedpuppy)
Attachment #9007039 - Flags: approval-mozilla-beta?
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
Attachment #9007039 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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!
Flags: needinfo?(mixedpuppy)
The OS where I reproduced these scenarios was Win 7 64-bit with the extension from the Description.
(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
Flags: needinfo?(mixedpuppy) → needinfo?(felipc)
Attached file 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
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
Flags: needinfo?(felipc)
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.
Attachment #9008793 - Flags: review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f4476938f7d7
followup fix to enable the click handler actor in sidebar, r=mconley
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
Attachment #9008793 - Flags: approval-mozilla-beta?
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.
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/42174b72eda5
Backed out changeset f4476938f7d7 for browser chrome failures on browser_address_edit. CLOSED TREE
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"}
Flags: needinfo?(mixedpuppy)
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/00fa9c00facf
followup fix to enable the click handler actor in sidebar, r=mconley
Flags: needinfo?(mixedpuppy)
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.
Attachment #9008793 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Attached image 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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Attached image 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.
Flags: needinfo?(mixedpuppy)
The scenarios from above are not working in the normal or pinned tabs.
Probably because the second patch has not landed on b7 yet.
Flags: needinfo?(mixedpuppy) → needinfo?(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.
Flags: needinfo?(pascalc)
Retested all the scenarios on 63.0b8(20180920135444) under Win 7 64-bit and Mac OS X 10.13.3 and everything looks good.
You need to log in before you can comment on or make changes to this bug.