Closed Bug 460146 Opened 16 years ago Closed 12 years ago

CSS3 border-image images should appear in Page Info -> Media

Categories

(Firefox :: Page Info Window, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 13

People

(Reporter: zwol, Assigned: iorsh)

References

(Depends on 1 open bug, )

Details

Attachments

(1 file, 5 obsolete files)

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).
Flags: wanted-firefox3.1?
Flags: wanted-firefox3.5?
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?
Attachment #584932 - Flags: review?
Attachment #584932 - Flags: review?(gavin.sharp)
Attachment #584932 - Flags: review?
Attachment #584932 - Flags: feedback?(mozilla.bugs)
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).
Attachment #584932 - Flags: feedback?(mozilla.bugs) → feedback+
> +    // 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.
(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.
(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...
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.
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.
Assignee: nobody → iorsh
Status: NEW → ASSIGNED
Supported -moz-image-rect, fixed TODO comments as requested
Attachment #584932 - Attachment is obsolete: true
Attachment #587074 - Flags: feedback?(mozilla.bugs)
Attachment #584932 - Flags: review?(gavin.sharp)
Filed Bug 714757 for -moz-image-rect scripting
Depends on: 713643
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.
Depends on: 714757
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 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.
Attachment #587074 - Flags: feedback?(mozilla.bugs) → feedback+
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.
Attachment #587074 - Attachment is obsolete: true
Attachment #587438 - Flags: feedback?(mozilla.bugs)
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.
Attachment #587438 - Flags: feedback?(mozilla.bugs) → feedback+
Attachment #584932 - Attachment description: Patch to add border images, list bullets, cursors to Media tab → Patch to add border images, list bullets, cursors to Media tab v1
Attachment #587074 - Attachment description: Patch to add border images, list bullets, cursors to Media tab → Patch to add border images, list bullets, cursors to Media tab v2
Attachment #587438 - Attachment description: Patch to add border images, list bullets, cursors to Media tab → Patch to add border images, list bullets, cursors to Media tab v3
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.
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.
Addressed latest comments
Attachment #587438 - Attachment is obsolete: true
Attachment #587802 - Flags: review?(db48x)
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.
Attachment #587802 - Flags: review?(db48x) → review-
Fixed, addImgFunc() now accepts two arguments instead of context.
Attachment #587802 - Attachment is obsolete: true
Attachment #588174 - Flags: review?(db48x)
db48x: Do the changes in the new patch fix everything to your satisfaction?
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
Attachment #588174 - Flags: review?(db48x) → review+
Attachment #588174 - Flags: checkin?
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
Attachment #588174 - Flags: checkin? → checkin+
Flags: in-testsuite+
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.
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?
Depends on: 722862
(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.
Have you tried changing computedStyle.getPropertyCSSValue("-moz-border-image") to computedStyle.getPropertyCSSValue("-moz-border-image-source") in grabAll()?
used "-moz-border-image-source" per @zwol, seems to be working.
Attachment #588174 - Attachment is obsolete: true
Attachment #593921 - Flags: review?(db48x)
Attachment #593921 - Flags: checkin?(db48x)
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)
Attachment #593921 - Flags: review+
Whiteboard: [autoland-try]
Whiteboard: [autoland-try] → [autoland-in-queue]
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
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
Whiteboard: [autoland-in-queue]
Is there need for any further action on my part? The test results look rather cryptic to me...
Attachment #593921 - Flags: review?(db48x) → review+
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
Target Milestone: --- → Firefox 13
Attachment #593921 - Flags: checkin?(db48x) → checkin+
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

:-)
(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... ;)
Thank you! I have some issues in mind in this area.
https://hg.mozilla.org/mozilla-central/rev/54812b404796
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.