Closed Bug 1326626 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Inspector: Rules, defect, P2)

Product:
DevTools
Component:
Inspector: Rules
Type:
defect

Tracking

(firefox54 verified, firefox55 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox54 --- verified
firefox55 --- verified

People

(Reporter: arni2033, Assigned: brennan.brisad, Mentored)

References

(
URL
)

Details

Attachments

(1 file, 1 obsolete file)

bug1326626-2.patch
3.44 KB, patch
jdescottes
: review+
gchang
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160511030221
STR_1:
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
No longer blocks: 1277113
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
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
Status: NEW → ASSIGNED
Attached patch bug1326626.patch (obsolete) — — Splinter 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]
bug1326626.patch

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: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=709c025d50460b382a47fe6f770180dddb7f7dcc

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.
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
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/addbb183e131
Open rule link in new tab also when devtools is in separate window. r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/addbb183e131
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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]
bug1326626-2.patch

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?
Comment on attachment 8846351 [details] [diff] [review]
bug1326626-2.patch

Fix an UI issue in devtools and also include tests. Aurora54+.
Attachment #8846351 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
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: RESOLVED → VERIFIED
status-firefox55: fixed → verified
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+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: