1310310 - Find an alternative to XUL:label for the stylesheet links

Status: VERIFIED FIXED

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

Description

[Julian Descottes [:jdescottes]]

This bug is blocking Bug 1291049.

The rule view is still relying on a XUL label for the stylesheet links. They are using the crop=center attribute in order to have a centered dynamic ellipsis.

In order to load the inspector in a content tab, we must remove all remaining XUL elements, so we should investigate an HTML based alternative.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: ruleview-xul-label.png

Comment 2

[Julian Descottes [:jdescottes]]

I found the following threads/docs about the possibility of adding middle cropping features to the text-overflow CSS property:
- https://www.w3.org/wiki/Text-overflow_middle_cropping
- Bug 740910

In the meantime we should try to find an alternative.

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1310310 - ruleview stylesheet link using span instead of label;

Review commit: https://reviewboard.mozilla.org/r/86114/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86114/

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8801334 [details]
Bug 1310310 - ruleview stylesheet link using span instead of label;

This patch is a CSS only workaround using the usual RTL workaround to crop the link at the beginning.

I think we can consider the "end" part of a stylesheet link to be the most meaningful. Still not as good as middle cropping maybe though.

If this is not ok, we can either look at JS alternatives or maybe have different implementations when in content and in chrome.

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1310310 - fixing tests

Review commit: https://reviewboard.mozilla.org/r/86116/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86116/

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8801334 [details]
Bug 1310310 - ruleview stylesheet link using span instead of label;

https://reviewboard.mozilla.org/r/86114/#review84728

::: devtools/client/themes/rules.css:146
(Diff revision 1)
> +  /* Workaround to crop the source link at the beginning. */
> +  overflow: hidden;
> +  text-overflow: ellipsis;
> +  direction: rtl;

I would direction: rtl just below the comment because it's more relevant to it.

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8801337 [details]
Bug 1310310 - fixing tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86116/diff/1-2/

Comment 8

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8801334 [details]
Bug 1310310 - ruleview stylesheet link using span instead of label;

https://reviewboard.mozilla.org/r/86114/#review84916

::: devtools/client/themes/rules.css:146
(Diff revision 1)
> +  /* Workaround to crop the source link at the beginning. */
> +  overflow: hidden;
> +  text-overflow: ellipsis;
> +  direction: rtl;

The RTL direction can break the formatting of text inside the .ruleview-rule-source element. See bug 1290056 to see what can happen and how to fix it.

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8801334 [details]
Bug 1310310 - ruleview stylesheet link using span instead of label;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86114/diff/1-2/

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8801337 [details]
Bug 1310310 - fixing tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86116/diff/2-3/

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8801334 [details]
Bug 1310310 - ruleview stylesheet link using span instead of label;

The changes look good to me. How does this behave in an RTL browser though? Does it still crop the source at the beginning?
I also agree that keeping the end of the URL visible is the most useful we can do under the circumstances.

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: stylesheet-link-rtl.png

(In reply to Patrick Brosset <:pbro> from comment #11)
> Comment on attachment 8801334 [details]
> Bug 1310310 - ruleview stylesheet link using span instead of label;
> 
> The changes look good to me. How does this behave in an RTL browser though?
> Does it still crop the source at the beginning?

Works fine in RTL direction as well. The direction: rtl; on `.ruleview-rule-source` becomes redundant with the inherited value, but the behavior remains the same. Screenshot attached. 

> I also agree that keeping the end of the URL visible is the most useful we
> can do under the circumstances.

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 8801334 [details]
Bug 1310310 - ruleview stylesheet link using span instead of label;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86114/diff/2-3/

Comment 14

[Julian Descottes [:jdescottes]]

Comment on attachment 8801334 [details]
Bug 1310310 - ruleview stylesheet link using span instead of label;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86114/diff/3-4/

Comment 15

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8801334 [details]
Bug 1310310 - ruleview stylesheet link using span instead of label;

https://reviewboard.mozilla.org/r/86114/#review85520

Looks and works great, thanks! It's a shame that left-side cropping needs to be this complicated in CSS and that crop:center is not possible at all.

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8801334 [details]
Bug 1310310 - ruleview stylesheet link using span instead of label;

https://reviewboard.mozilla.org/r/86114/#review84728

Thanks for having a look Tim.

> I would direction: rtl just below the comment because it's more relevant to it.

Fixed, good point!

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8801334 [details]
Bug 1310310 - ruleview stylesheet link using span instead of label;

https://reviewboard.mozilla.org/r/86114/#review84916

Thanks for the review Jarda! Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=45e96df694571fd5ac155b623a48a99185cfe9b5

Comment 18

[Julian Descottes [:jdescottes]]

Try looks green after getting a lot of unrelated "1266624 Intermittent-infra [test-linux.sh:error] failed to download mozharness zip". Landing.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b5f08127448
ruleview stylesheet link using span instead of label;r=jsnajdr

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8b5f08127448

Comment 21

[Julian Descottes [:jdescottes]]

Some pointers to verify this bug!

This bug modified the way stylesheet links are rendered in the inspector rule-view tab (see attachment 8801313 [details]). 

We should perform a non-regression on the display of those links. Try to visit websites with stylesheet links of different length, play with the available width for the rule view in order to force overflows, ellipses etc...

Please note that there is one known difference/regression compared to the previous behavior: the ellipsis used to appear in the middle of the link. It now appears on the left-side. This is a technical limitation and we won't be able to change this.

Comment 22

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

Thank you Julian Descottes for the guideline!

I reproduced this bug using Fx 52.0a1, build ID: 20161014060324, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 52.0a1, build ID: 20161113030203, on Windows 10 x64, Mac OS X 10.12 and Ubuntu 14.04 LTS.

Cheers!