bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th, from 16:00 until 20:00 UTC.

Clicking background url doesn't work if devtools were initially opened in separate window

VERIFIED FIXED in Firefox 54



Developer Tools: CSS Rules Inspector
a year ago
11 months ago


(Reporter: arni2033, Assigned: Michael Hoffmann, Mentored)


Firefox 55

Firefox Tracking Flags

(firefox54 verified, firefox55 verified)




(1 attachment, 1 obsolete attachment)



a year ago
>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160511030221
1. Open   data:text/html,<style>body{background:url(about:logo) no-repeat}
2. Open devtools in separate window, close devtools
3. Open inspector, select <body>, open ruleview, hover mouse over string "about:logo" in ruleview
4. Click first time to hide tooltip (see bug 1201831)
5. Click to open url in a new tab

AR:  No visible action
ER:  Url should open in a new tab, just like in devtools docked to the bottom side of window


a year ago
No longer blocks: 1277113


a year ago
Component: Untriaged → Developer Tools: CSS Rules Inspector
Basically, if devtools started in a separate window, links from the rule view do not open.

The issue comes from text-property-editor http://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/views/text-property-editor.js#277

The text-property-editor tries to open the URL in this.browserWindow which is initialized to this.doc.defaultView.top in the panel init. In case of devtools opened in a separate window, this will point to the devtools window.

Compared to the computed view implementation: 
> let browserWin = this.inspector.target.tab.ownerDocument.defaultView;
> browserWin.openUILinkIn(target.href, "tab");

which always gets the correct window.

The fix should be pretty simple to implement, but adding a test would be nice here.
Mentor: jdescottes
Priority: -- → P2

Comment 2

a year ago
Hi, I'd like to work on this bug.
Thanks Michael! Assigning the bug to you! 

For the test, we need to: 
- configure the devtools to open in a separate window
- open devtools for the test page [1]
- select body
- click on the link
- check that a new tab was opened on the expected URL

I'll try to find some reference tests to look at to implement this.

[1] We can't really use about:logo as a url for the test here. Let's use the following test url (the base64 png is a 1px black square):

data:text/html,<style>body{background:url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVQImWNgYGD4DwABBAEAfbLI3wAAAABJRU5ErkJggg==) no-repeat}
Assignee: nobody → brennan.brisad

Comment 4

a year ago
Created attachment 8846129 [details] [diff] [review]

Thanks Julian!

I looked at some existing tests and wrote a new one based on that, and used the code that you mentioned in order to fix the bug.
Attachment #8846129 - Flags: feedback?(jdescottes)
Comment on attachment 8846129 [details] [diff] [review]

Review of attachment 8846129 [details] [diff] [review]:

Great work Michael, fixes the issue and the text is on point.
Switching to r+ directly. 

A few nits for you to look at, let's also wait for the feedback from try: 

By the way, do you have a commit access lvl1 to push changesets on try?
If not, we definitely should request it :)

::: devtools/client/inspector/rules/test/browser.ini
@@ +229,5 @@
>  [browser_rules_strict-search-filter_02.js]
>  [browser_rules_strict-search-filter_03.js]
>  [browser_rules_style-editor-link.js]
>  [browser_rules_urls-clickable.js]
> +[browser_rules_url-click-opens-new-tab.js]

nit: this should be on the line above

::: devtools/client/inspector/rules/test/browser_rules_url-click-opens-new-tab.js
@@ +6,5 @@
> +
> +// Bug 1326626 - Tests that clicking a background url opens a new tab
> +// even when the devtools is opened in a separate window.
> +
> +var { Toolbox } = require("devtools/client/framework/toolbox");

Let's skip importing the Toolbox here and simply use "window" in openInspectorForURL.
Attachment #8846129 - Flags: feedback?(jdescottes) → review+
(In reply to Julian Descottes [:jdescottes] from comment #5)
> Comment on attachment 8846129 [details] [diff] [review]
> Great work Michael, fixes the issue and the text is on point.

s/text/test ...

> A few nits for you to look at, let's also wait for the feedback from try: 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=709c025d50460b382a47fe6f770180dddb7f7dcc

Some builds are failing, but it seems totally unrelated to your patch.

Comment 7

a year ago
Created attachment 8846351 [details] [diff] [review]

Great!  I've uploaded a new patch with the nits addressed.

I believe I do have commit access to try, but it seems to have been disabled due to inactivity.  I just created a request, Bug 1346596, to restore access :-)
Attachment #8846129 - Attachment is obsolete: true
Flags: needinfo?(jdescottes)
Thanks again for the new patch Michael, adding checkin-needed.
Flags: needinfo?(jdescottes)
Keywords: checkin-needed

Comment 9

a year ago
Pushed by ryanvm@gmail.com:
Open rule link in new tab also when devtools is in separate window. r=jdescottes
Keywords: checkin-needed

Comment 10

a year ago
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 11

a year ago
This bug is found in my system also when the background URL is opened while the Dev tools were opened in the window.which makes us very difficult to use the page's Dev tools and css elements of that particular page. which makes it so difficult when the developers are using it.This error is repeated rarely.
Should we uplift this?  Looks like it was reported in 49?
Flags: needinfo?(jdescottes)
Comment on attachment 8846351 [details] [diff] [review]

Approval Request Comment
[Feature/Bug causing the regression]: N/A, old bug which was reported only recently
[User impact if declined]: devtools users can not open image links from some devtools panels, if devtools is open in separate window.
[Is this code covered by automated tests?]: yes, added in this changeset
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: already covered by automated tests, so I would say no here (otherwise STRs from the summary can be used)
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple JS change, which is just reusing the same approach as in another panel
[String changes made/needed]: none

I tried grafting the changeset from central on aurora: it applies cleanly, test still passes.
Flags: needinfo?(jdescottes)
Attachment #8846351 - Flags: approval-mozilla-aurora?
status-firefox54: --- → affected
Comment on attachment 8846351 [details] [diff] [review]

Fix an UI issue in devtools and also include tests. Aurora54+.
Attachment #8846351 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+

Comment 15

a year ago
status-firefox54: affected → fixed
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

I was able to reproduce the issue using the STR from comment 0 on an old Nightly build 54.0a1 (2017-03-01).
The fix was verified on the latest Nightly 55.0a1 (2017-03-26) across platforms: 
- Windows 10 x64;
- Ubuntu 16.04 LTS x64;
- MAC OS X 10.12.4.
status-firefox55: fixed → verified

Comment 17

11 months ago
I have reproduced this issue using Firefox 49 (2016.05.11, build ID=20160511030221) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 54.0b4 on Win 8.1 x64, Mac OS X 10.11 and Ubuntu 16.04 x64.
status-firefox54: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.