Last Comment Bug 460146 - CSS3 border-image images should appear in Page Info -> Media
: CSS3 border-image images should appear in Page Info -> Media
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Page Info Window (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 13
Assigned To: Maxim Iorsh
:
Mentors:
http://www.css3.info/preview/border-i...
: 572173 (view as bug list)
Depends on: 714757 713643 722862
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-15 15:47 PDT by Zack Weinberg (:zwol)
Modified: 2012-05-30 23:25 PDT (History)
8 users (show)
mozilla.bugs: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to add border images, list bullets, cursors to Media tab v1 (3.69 KB, patch)
2011-12-30 01:28 PST, Maxim Iorsh
mozilla.bugs: feedback+
Details | Diff | Splinter Review
Patch to add border images, list bullets, cursors to Media tab v2 (4.05 KB, patch)
2012-01-09 12:18 PST, Maxim Iorsh
mozilla.bugs: feedback+
Details | Diff | Splinter Review
Patch to add border images, list bullets, cursors to Media tab v3 (7.19 KB, patch)
2012-01-10 12:57 PST, Maxim Iorsh
mozilla.bugs: feedback+
Details | Diff | Splinter Review
Patch to add border images, list bullets, cursors to Media tab v4 (7.08 KB, patch)
2012-01-11 13:25 PST, Maxim Iorsh
db48x: review-
Details | Diff | Splinter Review
Patch to add border images, list bullets, cursors to Media tab v5 (7.07 KB, patch)
2012-01-12 13:37 PST, Maxim Iorsh
db48x: review+
db48x: checkin+
Details | Diff | Splinter Review
Patch to add border images, list bullets, cursors to Media tab v6 (7.08 KB, patch)
2012-02-02 11:41 PST, Maxim Iorsh
db48x: review+
emorley: review+
emorley: checkin+
Details | Diff | Splinter Review

Description Zack Weinberg (:zwol) 2008-10-15 15:47:50 PDT
FF3.1 has support for the CSS3 border-image: property (see <http://dev.w3.org/csswg/css3-background/#the-border-image>) which lets you use an image for an element's border (cleverly sliced and diced and scaled and replicated).  Live examples are at the URL in the bug header.

Such images should be accessible through Page Info -> Media, but currently are not (tested with Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b1pre) Gecko/20081015 Firefox/3.1b2pre).
Comment 1 Tanner M. Young [:tmyoung] 2010-06-15 12:15:32 PDT
*** Bug 572173 has been marked as a duplicate of this bug. ***
Comment 2 Maxim Iorsh 2011-12-30 01:28:59 PST
Created attachment 584932 [details] [diff] [review]
Patch to add border images, list bullets, cursors to Media tab v1

Code: CSSValue returned by getPropertyCSSValue() has a very different structure for each case.

For bg images: value_list->url, for bullets: url, for cursors: value_list->value_list->value_list->url (!), thus needed generic recursion.

Also, tried to add test by copypasting browser/base/content/test/browser_pageInfo.js, but it doesn't seem to run at all.

Ran tests on local machine, got some seemimgly unrelated failures, such as 
js/src/jit-test/tests/sunspider/check-date-format-tofte.js. Couldn't determine whether this is my problem, or just destabilized tree. Would it be possible to submit patch to try server?
Comment 3 Tanner M. Young [:tmyoung] 2011-12-30 10:53:58 PST
Comment on attachment 584932 [details] [diff] [review]
Patch to add border images, list bullets, cursors to Media tab v1

Looks excellent to me.  I have no way of testing the patch in context, but I like your approach to it.

My only question is will this handle cases where urls are children of other values like -moz-image-rect:

.foo {background-image: -moz-image-rect( url('link.png'), 0, 32, 26, 0)}

Also we should indicate on the bug to support border-image that someone needs to come back and uncomment that line when that feature is available (or file a new bug about the change that depends on that one).
Comment 4 Philip Chee 2011-12-30 11:02:20 PST
> +    // addImgFunc.bind(gStrings.mediaBorderImg)(computedStyle.getPropertyCSSValue("border-image")); // unsupported
In general commented out code is dis-recommended. Instead add a comment to use "border-image" when it is implemented.
Comment 5 Tanner M. Young [:tmyoung] 2011-12-30 11:06:36 PST
(In reply to Maxim Iorsh from comment #2)
> Also, tried to add test by copypasting
> browser/base/content/test/browser_pageInfo.js, but it doesn't seem to run at
> all.

So you tried to create a new test that didn't start or did the old or new test not work?

> Ran tests on local machine, got some seemingly unrelated failures, such as 
> js/src/jit-test/tests/sunspider/check-date-format-tofte.js. Couldn't
> determine whether this is my problem, or just destabilized tree. Would it be
> possible to submit patch to try server?

My guess is that this is Bug 600522 or Bug 515254.
Comment 6 Tanner M. Young [:tmyoung] 2011-12-30 11:08:15 PST
(In reply to Philip Chee from comment #4)
> > +    // addImgFunc.bind(gStrings.mediaBorderImg)(computedStyle.getPropertyCSSValue("border-image")); // unsupported
> In general commented out code is dis-recommended. Instead add a comment to
> use "border-image" when it is implemented.

Agreed.  I meant to say something more like that (as it's how we've done it with Page Info in the past): I just can't think today...
Comment 7 Maxim Iorsh 2012-01-02 01:52:22 PST
Patch from 2011-12-30 doesn't support -moz-image-rect. In fact, getPropertyCSSValue() returns a primitive string value in this case, which need to be regexp-parsed further.

What do you think would be better? One possiblity is to file a separate bug to enable scriptability of -moz-image-rect and similar CSS values, and the other is to parse the string independently.
Comment 8 Tanner M. Young [:tmyoung] 2012-01-02 12:08:14 PST
My suggestion is to do both: you should file the bug (and please note the bug number here so we can cc ourselves if we want), and then fix the issue locally so that you are not waiting on the other bug to be fixed before you can submit a patch for this one.  With Bug 519390, I'm still waiting on Bug 522850 to be fixed, and that was over two years ago.
Comment 9 Maxim Iorsh 2012-01-09 12:18:52 PST
Created attachment 587074 [details] [diff] [review]
Patch to add border images, list bullets, cursors to Media tab v2

Supported -moz-image-rect, fixed TODO comments as requested
Comment 10 Maxim Iorsh 2012-01-09 12:22:57 PST
Filed Bug 714757 for -moz-image-rect scripting
Comment 11 Maxim Iorsh 2012-01-09 12:26:42 PST
I wanted to modify browser/base/content/test/browser_pageInfo.js to check the Media tab too, but the Media tab is filled in a separate thread, so the existing "page-info-dialog-loaded" observer couldn't catch it. Any hint about how to define correct observer or any other method to test Media tab would be appreciated.
Comment 12 Tanner M. Young [:tmyoung] 2012-01-09 13:11:11 PST
The test for View Image Info (and subsequent changes to it) is /browser/base/content/test/browser_bug517902.js

I can't remember why we didn't--or couldn't--use browser/base/content/test/browser_pageInfo.js, but we ended up making a new test altogether.  (I think we needed to interact with Page Info in a manner that was somehow impossible because of the structure of the other bug, or we wanted it clear which bug we were testing, it's been long enough I'm really not sure.)
Comment 13 Tanner M. Young [:tmyoung] 2012-01-09 13:44:03 PST
Comment on attachment 587074 [details] [diff] [review]
Patch to add border images, list bullets, cursors to Media tab v2

I like the changes, but I don't know if we are dealing with a parsed value for -moz-image-rect or worse the raw value from the file (I don't know since I can't test it).  If it is the raw value, we need handle a lot more special cases, like those below.  Please test this patch to make sure that the following url formatting still works (both with relative and absolute urls).  Otherwise, we will need to add a lot more to the regular expression.  (I sincerely hope we do not.)

url("foo.jpg")
url(    "foo.jpg"    )
url('foo.jpg')
url(    'foo.jpg'    )
url(foo.jpg)
url(   foo.jpg    )

etc.

Also, when working with the test, be advised that /browser/base/content/test/browser_bug517902.js tests between 5 and 10 other bugs looking for very specific regressions--related to the order, size, and origin of image(s), and minor changes to the test may accidentally mask those regressions.  I would recommend either studying the history of that file very carefully to ensure that no special case is harmed, or clone the test--marking a new one for this bug.  (If you do the latter, you will need to add it to the list in /browser/base/content/test/Makefile.in)  See https://bug517902.bugzilla.mozilla.org/attachment.cgi?id=409534 and http://hg.mozilla.org/mozilla-central/annotate/c713003d3226/browser/base/content/test/browser_bug517902.js

Otherwise, things look very good to me.  If the regular expression doesn't need to be changed, I would recommend creating a test and then requesting a review--I would recommend db48x doing the review, if he is around.
Comment 14 Maxim Iorsh 2012-01-10 12:57:00 PST
Created attachment 587438 [details] [diff] [review]
Patch to add border images, list bullets, cursors to Media tab v3

Tested url(    "foo.jpg"    ), works ok. Indeed getPropertyCSSValue() returns style in a parsed form, so no problem here. Absolute/relative urls both work fine too.

Created test browser_bug460146.js, didn't want to throw unrelated issues into one bin.
Comment 15 Tanner M. Young [:tmyoung] 2012-01-10 15:48:31 PST
Comment on attachment 587438 [details] [diff] [review]
Patch to add border images, list bullets, cursors to Media tab v3

I think we can remove the two commented lines in the test that are leftover from the original: we had them to check for specific images in the list in the same manner that View Image Info does, since View Image Info cannot be tested directly through the context menu.

Have we run the test successfully?  If memory serves, imagetree consolidates list items so it's possible that there will only be four items in imagetree with one having four instances (which happens when I modify the test to only use background-image).  If that is the case for this case, we should use multiple images rather than about:logo, so that we don't need to change the javascript of the test, since testing for the total number of instances of about:logo would be more complicated.
Comment 16 Maxim Iorsh 2012-01-11 01:43:32 PST
I don't quite understand your last comment. Do you mean that the imagetree can collapse list when there is not enough memory? Right now the test passes, as the same image about:logo is listed separately for each media type (Background vs border vs bullet vs cursor).

I can use about:logo?a, about:logo?b, about:logo?c etc. just to be on the safe side, if you wish.
Comment 17 Tanner M. Young [:tmyoung] 2012-01-11 10:16:47 PST
Okay.  I forgot that the only time that Page Info doesn't collapse duplicate entries is when they are of the same type.  However, it's possible that we might change that in future to have Page Info not list duplicate resources at all (which I might recommend but not in this bug), but I'd like more input from others on the pros and cons of that.  Therefore, to be safe, go ahead and use about:logo?a, about:logo?b, about:logo?c, etc. in case that changes someday.  Sorry about the confusion and ambiguity there.

Otherwise, I think everything is ready for an official review.
Comment 18 Maxim Iorsh 2012-01-11 13:25:25 PST
Created attachment 587802 [details] [diff] [review]
Patch to add border images, list bullets, cursors to Media tab v4

Addressed latest comments
Comment 19 Daniel Brooks [:db48x] 2012-01-11 17:40:03 PST
Comment on attachment 587802 [details] [diff] [review]
Patch to add border images, list bullets, cursors to Media tab v4

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

This is a good patch, but there is one relatively minor thing I'd like you to change.

::: browser/base/content/pageinfo/pageInfo.js
@@ +622,3 @@
>    if (computedStyle) {
> +    var addImgFunc = function (val) {
> +      var label = this; // CONTEXT!!!

The fact that you need this shouty comment is a red flag. Instead of passing the label in as "this", just make it the first of two arguments. Using Function.bind is fun, but not needed in this case, since the function never leaves its environment.

@@ +644,5 @@
> +    addImgFunc.bind(gStrings.mediaBGImg)(computedStyle.getPropertyCSSValue("background-image"));
> +    addImgFunc.bind(gStrings.mediaBorderImg)(computedStyle.getPropertyCSSValue("-moz-border-image"));
> +    // TODO: support unprefixed "border-image" once bug 713643 is fixed.
> +    addImgFunc.bind(gStrings.mediaListImg)(computedStyle.getPropertyCSSValue("list-style-image"));
> +    addImgFunc.bind(gStrings.mediaCursor)(computedStyle.getPropertyCSSValue("cursor"));

which makes these lines simpler:

addImgFunc(gStrings.mediaBGImg, computedStyle.getPropertyCSSValue("background-image"));

and so on. Not as exciting, but easier to read.
Comment 20 Maxim Iorsh 2012-01-12 13:37:40 PST
Created attachment 588174 [details] [diff] [review]
Patch to add border images, list bullets, cursors to Media tab v5

Fixed, addImgFunc() now accepts two arguments instead of context.
Comment 21 Tanner M. Young [:tmyoung] 2012-01-19 00:23:18 PST
db48x: Do the changes in the new patch fix everything to your satisfaction?
Comment 22 Daniel Brooks [:db48x] 2012-01-22 23:35:43 PST
Comment on attachment 588174 [details] [diff] [review]
Patch to add border images, list bullets, cursors to Media tab v5

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

Thank you Maxim; my apologies for the delay. r=db48x
Comment 23 Daniel Brooks [:db48x] 2012-01-25 19:10:43 PST
Comment on attachment 588174 [details] [diff] [review]
Patch to add border images, list bullets, cursors to Media tab v5

http://hg.mozilla.org/integration/mozilla-inbound/rev/2e37e7223aa3
Comment 25 Maxim Iorsh 2012-01-27 05:28:28 PST
It seems somebody just broke -moz-border-image (possibly this patch: http://hg.mozilla.org/mozilla-central/rev/dda1146d1ec3). See example: http://www.w3schools.com/cssref/tryit_view.asp?filename=trycss3_border-image - doesn't work in 27-Jan-2012 nightly build.
Comment 26 Maxim Iorsh 2012-01-31 13:39:50 PST
Any ideas about what should be done now? I can remove the "border-image" line from the patch, but then it will fix anything but the very reported issue. This would look quite bizarre, I think. Shall I open yet another bug or try to have bug 497995 reopened?
Comment 27 Tanner M. Young [:tmyoung] 2012-01-31 14:11:21 PST
(In reply to Maxim Iorsh from comment #26)
> Any ideas about what should be done now? I can remove the "border-image"
> line from the patch, but then it will fix anything but the very reported
> issue. This would look quite bizarre, I think. Shall I open yet another bug
> or try to have bug 497995 reopened?

I've filed Bug 722862 to report the behavior you're seeing here after speaking to khuey about the patch for Bug 497995.  He was simply grooming another developer's patch and doesn't have a lot of information on what would have broken this, but David Baron should be able to help us, but he is away for another two weeks.  I have CC'ed both of them on that bug, and we'll probably have a much better idea of what to do upon his return.
Comment 28 Zack Weinberg (:zwol) 2012-01-31 14:14:12 PST
Have you tried changing computedStyle.getPropertyCSSValue("-moz-border-image") to computedStyle.getPropertyCSSValue("-moz-border-image-source") in grabAll()?
Comment 29 Maxim Iorsh 2012-02-02 11:41:16 PST
Created attachment 593921 [details] [diff] [review]
Patch to add border images, list bullets, cursors to Media tab v6

used "-moz-border-image-source" per @zwol, seems to be working.
Comment 30 Ed Morley [:emorley] 2012-02-04 09:48:42 PST
Comment on attachment 593921 [details] [diff] [review]
Patch to add border images, list bullets, cursors to Media tab v6

(To keep autoland happy about permissions)
Comment 31 Mozilla RelEng Bot 2012-02-04 09:53:59 PST
Autoland Patchset:
	Patches: 593921
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/rev/605242756469
Try run started, revision 605242756469. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=605242756469
Comment 32 Mozilla RelEng Bot 2012-02-04 14:15:20 PST
Try run for 605242756469 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=605242756469
Results (out of 207 total builds):
    success: 184
    warnings: 20
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-605242756469
Comment 33 Maxim Iorsh 2012-02-04 22:51:30 PST
Is there need for any further action on my part? The test results look rather cryptic to me...
Comment 34 Ed Morley [:emorley] 2012-02-05 11:10:48 PST
Hi! No action needed - the only oranges & reds (test failures) on try were due to known intermittently failing tests/bad test slaves, so were not caused by this patch.

I've therefore pushed this to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54812b404796

Thanks for the patch! It will be merged to mozilla-central by one of the sheriffs within 24 hours and in the first Nightly generated after that.

Join us on irc (#developers on irc.mozilla.org) and we'll find some other things for you to work on if you are interested? :-D
Comment 35 Ed Morley [:emorley] 2012-02-05 11:43:13 PST
Meant to say for future patches could you set your hgrc to include the author automatically & also add a commit message, along the lines of:
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

:-)
Comment 36 Tanner M. Young [:tmyoung] 2012-02-05 23:23:33 PST
(In reply to Ed Morley [:edmorley] from comment #34)
> Join us on irc (#developers on irc.mozilla.org) and we'll find some other
> things for you to work on if you are interested? :-D

I'd be happy to point you towards a list of all of my unfinished Page Info patches and save you a visit to #developers... ;)
Comment 37 Maxim Iorsh 2012-02-05 23:27:02 PST
Thank you! I have some issues in mind in this area.
Comment 38 Marco Bonardo [::mak] 2012-02-06 00:50:12 PST
https://hg.mozilla.org/mozilla-central/rev/54812b404796

Note You need to log in before you can comment on or make changes to this bug.