677930 - Style Inspector: make URLs clickable

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Mihai Sucan [:msucan]]

When an image URL or some other URL is displayed in the Style Inspector we should allow the user to click the address to load the resource.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Bug triage, filter on PEGASUS.

Comment 2

[briangrinstead]

I'd like to work on this item as a way to learn how to commit to developer tools.

I'll attach a patch - what else do I need to know or do?  Should I also add a test case in styleinspector/test?

Comment 3

[briangrinstead]

Attachment: Initial patch adding ability to URLs clickable in the style inspector

Comment 4

[Heather Arthur [:harth]]

(In reply to Brian Grinstead from comment #2)
> I'll attach a patch - what else do I need to know or do?  Should I also add
> a test case in styleinspector/test?

I would make sure that the style inspector tests pass, and add a test for this too. After that you can flag someone for review, Dave Camp (:dcamp) would be a good reviewer for this area.

Ask in irc.mozilla.org #devtools if you have any questions about this or running the tests. Thanks for working on this, it's been much needed.

Comment 5

[briangrinstead]

Attachment: Added a basic test case, ensure that existing styleinspector tests are passing

Comment 6

[briangrinstead]

(In reply to Heather Arthur [:harth] from comment #4)

> I would make sure that the style inspector tests pass, and add a test for
> this too. After that you can flag someone for review, Dave Camp (:dcamp)
> would be a good reviewer for this area.

I have made a few changes to make sure that existing tests are working, and added a test file here: browser/devtools/styleinspector/test/browser_styleinspector_bug_677930_urls_clickable.js.  This test is not yet actually checking for the links, though.

> Ask in irc.mozilla.org #devtools if you have any questions about this or
> running the tests.

I will probably need to ping you on irc to get a little more info about the tests.  I've been looking at browser_ruleview_ui.js and browser_ruleview_manipulation.js to try and figure out what would need to be done to test this.

Comment 7

[briangrinstead]

Attachment: Updating styleinspector test to check if links show up when background image is applied. Checks from inline styles, and absolute / relative paths from an external sheet

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Brian, does these need reviewing?

Comment 9

[Joe Walker [:jwalker]]

Comment on attachment 739372 [details] [diff] [review]
Updating styleinspector test to check if links show up when background image is applied.  Checks from inline styles, and absolute / relative paths from an external sheet

Review of attachment 739372 [details] [diff] [review]:
-----------------------------------------------------------------

https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch has details about how to mark a patch for review. Paul is on holiday for a break, so Mike/Heather or I are best for reviews right now.

::: browser/devtools/styleinspector/CssRuleView.jsm
@@ +26,5 @@
>  
> +// Used to parse an external resource from a property value
> +const CSS_RESOURCE_RE = /url\([\'\"]?(.*?)[\'\"]?\)/;
> +
> +const IOService = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService);

I hate myself every-time I say this because it sounds really picky, but could you try to stick to 80 chars please? :)

::: browser/devtools/styleinspector/ruleview.css
@@ +29,5 @@
>  }
>  
> +.ruleview-propertycontainer a {
> +  cursor: pointer;
> +  text-decoration: underline;

I think an underline is probably theme CSS, so we should store it in another file so Firefox themes can override it. See https://wiki.mozilla.org/DevTools/CSSTips#Content_or_Theme_CSS for more details.

::: browser/devtools/styleinspector/test/Makefile.in
@@ +52,5 @@
>    browser_bug705707_is_content_stylesheet_xul.css \
>    browser_bug722196_identify_media_queries.html \
> +  browser_styleinspector_bug_677930_urls_clickable.html \
> +  browser_styleinspector_bug_677930_urls_clickable \
> +  browser_styleinspector_bug_677930_urls_clickable/browser_styleinspector_bug_677930_urls_clickable.css \

Normally we don't create subdirectories per test. While it might make sense, I'm not sure it's worth bucking the trend, so could you flatten this out please.

Comment 10

[Joe Walker [:jwalker]]

(Also, I should have said - thanks for the patch!)

Comment 11

[briangrinstead]

(In reply to Joe Walker [:jwalker] from comment #9)
> Comment on attachment 739372 [details] [diff] [review]
> Updating styleinspector test to check if links show up when background image
> is applied.  Checks from inline styles, and absolute / relative paths from
> an external sheet
> 
> Review of attachment 739372 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> How_to_Submit_a_Patch has details about how to mark a patch for review. Paul
> is on holiday for a break, so Mike/Heather or I are best for reviews right
> now.
> 

Thanks, I will make the changes listed and then submit the updated patch for review.

Comment 12

[briangrinstead]

(In reply to Joe Walker [:jwalker] from comment #9)

> Normally we don't create subdirectories per test. While it might make sense,
> I'm not sure it's worth bucking the trend, so could you flatten this out
> please.

The reason I made a new subdirectory was to make sure that the relative image URLs were being resolved relative to the CSS file, not to the page.  I'm sure we can trust the IOService to do this correctly, but I wanted to cover this with the test in case we gathered the wrong base href for the sheet (either in CSSLogic.jsm or in the TextPropertyEditor).  I'd be happy to flatten it out if that is preferable, but I couldn't think of another way to test that particular case without a separate subdirectory.

Comment 13

[Joe Walker [:jwalker]]

(In reply to Brian Grinstead from comment #12)
> (In reply to Joe Walker [:jwalker] from comment #9)
> 
> > Normally we don't create subdirectories per test. While it might make sense,
> > I'm not sure it's worth bucking the trend, so could you flatten this out
> > please.
> 
> The reason I made a new subdirectory was to make sure that the relative
> image URLs were being resolved relative to the CSS file, not to the page.
> ...

Ah yes - that makes lots of sense, thanks.

Comment 14

[briangrinstead]

(In reply to Joe Walker [:jwalker] from comment #9)

> I think an underline is probably theme CSS, so we should store it in another
> file so Firefox themes can override it. See
> https://wiki.mozilla.org/DevTools/CSSTips#Content_or_Theme_CSS for more
> details.
> 

I'm not seeing anything in the theme that specifies an underline.  Do you think I should just remove the underline rule, keep the theme-link class and allow it to controlled by the theme (only has the underline on hover), or should I add a new theme class / update the theme to cause the underline to be on by default?  I notice that the `ruleview-rule-source` links do not have an underline, so maybe it would just fit better within the UI if these don't either?

Comment 15

[briangrinstead]

Attachment: Fixed style issues, ready for review

Thanks, and let me know if I'm doing this wrong.

Comment 16

[Joe Walker [:jwalker]]

Comment on attachment 739860 [details] [diff] [review]
Fixed style issues, ready for review

Review of attachment 739860 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the updated patch.

It's normal practice to stick with the same reviewer for a patch because anyone that's second to look at a patch is likely to have to repeat work done by someone else (I guess the exception is when different reviewer look at different pieces, but that's not the case here). So I'll relieve dcamp/mike.

I've taken another look at the code, and that all looks good to me. I'd like to build it and take it for a spin, but that's going to have to wait for Monday.

Comment 17

[Joe Walker [:jwalker]]

Comment on attachment 739860 [details] [diff] [review]
Fixed style issues, ready for review

Review of attachment 739860 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry it's taken a while to get to this, but it looks good to me.
I've pushed it to our test-runner (called 'try') and the results will come in here: https://tbpl.mozilla.org/?tree=Try&rev=6c6c531436d2
The browser that I built was quite crashy. One of the crashes came when I was clicking a link, but there were enough of them that I think it's unlikely that this is your fault. Try should tell us more.

Comment 18

[briangrinstead]

(In reply to Joe Walker [:jwalker] from comment #17)

> Sorry it's taken a while to get to this, but it looks good to me.
> I've pushed it to our test-runner (called 'try') and the results will come
> in here: https://tbpl.mozilla.org/?tree=Try&rev=6c6c531436d2
> The browser that I built was quite crashy. One of the crashes came when I
> was clicking a link, but there were enough of them that I think it's
> unlikely that this is your fault. Try should tell us more.

Great, let me know if there is anything else I can do.  By the way, I was wondering if there was a way to hook into the browser API to open the image directly in a new tab rather than having the <a> tag open the link in a new window using target _blank.  It seems like it may be more convenient to open it in a new tab so the window won't lose focus.

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Brian Grinstead from comment #18)
> By the way, I was
> wondering if there was a way to hook into the browser API to open the image
> directly in a new tab rather than having the <a> tag open the link in a new
> window using target _blank.  It seems like it may be more convenient to open
> it in a new tab so the window won't lose focus.

It is fairly simple to open a page in a new tab. Just search for mdnLinkClick to see an appropriate example.

Comment 20

[briangrinstead]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #19)

> It is fairly simple to open a page in a new tab. Just search for
> mdnLinkClick to see an appropriate example.

Oh cool, I see that now.  Looks easy enough.  I have two related questions:
1. Do you think it would be better to use mdnLinkClick / openUILinkIn as opposed to the <a target=_blank>?
2. If so, would you recommend I modify the existing patch to include this change, or would it be better to land a follow up patch with just that change?

Comment 21

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

- Using target="_blank" will open all links in the same tab (and open a new tab if _blank does not already exist).
- Using openUILinkIn would allow us to open multiple links in multiple tabs.

Personally, I would prefer each clicked link to open in a new tab.

Because this patch is already r+, if you decide that you want to use openUILinkIn you should probably do it as part of a new bug.

Comment 22

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=8911b764dc1e
https://hg.mozilla.org/integration/fx-team/rev/8911b764dc1e

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8911b764dc1e

Comment 24

[briangrinstead]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #21)
> - Using target="_blank" will open all links in the same tab (and open a new
> tab if _blank does not already exist).
> - Using openUILinkIn would allow us to open multiple links in multiple tabs.
> 
> Personally, I would prefer each clicked link to open in a new tab.
> 
> Because this patch is already r+, if you decide that you want to use
> openUILinkIn you should probably do it as part of a new bug.

I agree that opening the link in a new tab would be ideal. I'd like to land a follow up patch to change this behavior, what is the best way to handle this?  Should I follow the process of entering a new bug here: https://bugzilla.mozilla.org/enter_bug.cgi and fill it out with the same options as this one has?  Or is there a different step to do before opening it up as a case?

Comment 25

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Brian Grinstead from comment #24)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #21)
> > - Using target="_blank" will open all links in the same tab (and open a new
> > tab if _blank does not already exist).
> > - Using openUILinkIn would allow us to open multiple links in multiple tabs.
> > 
> > Personally, I would prefer each clicked link to open in a new tab.
> > 
> > Because this patch is already r+, if you decide that you want to use
> > openUILinkIn you should probably do it as part of a new bug.
> 
> I agree that opening the link in a new tab would be ideal. I'd like to land
> a follow up patch to change this behavior, what is the best way to handle
> this?  Should I follow the process of entering a new bug here:
> https://bugzilla.mozilla.org/enter_bug.cgi and fill it out with the same
> options as this one has?  Or is there a different step to do before opening
> it up as a case?

Yes, you could either do that or click "Clone This Bug" at the bottom right of this page and go from there.

Comment 26

[Luke]

The clickable url link is currently broken in some cases - see https://bugzilla.mozilla.org/show_bug.cgi?id=921686

A popup display of the image might be cool too.

Comment 27

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Luke from comment #26)
> The clickable url link is currently broken in some cases - see
> https://bugzilla.mozilla.org/show_bug.cgi?id=921686
> 
> A popup display of the image might be cool too.

We are working on that

Comment 28

[Brian Grinstead [:bgrins]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #27)
> (In reply to Luke from comment #26)
> > The clickable url link is currently broken in some cases - see
> > https://bugzilla.mozilla.org/show_bug.cgi?id=921686
> > 
> > A popup display of the image might be cool too.
> 
> We are working on that

The popup display of images is Bug 765105.