1078337 - Line next to find button in menu

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Wesley Johnston (:wesj)]

Attachment: find.png

Theres a tiny thin line next to the find button.

Comment 1

[Markus Stange [:mstange]]

If this regressed recently, it's probably due to bug 1044702.

Comment 2

[:Gijs (out for now; he/him)]

I can reproduce this, and yes, I'm fairly sure this is a recent regression.

Comment 3

[:Gijs (out for now; he/him)]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7c24470b6b3a&tochange=14665b1de5ee

Comment 4

[:Gijs (out for now; he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a61ddc0c62fd&tochange=6fb508e952b1

6fb508e952b1	Matt Woodrow — Bug 1070722 - Use the imagelib high quality downscaler on OSX instead of the quartz one. r=jrmuizel

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: regression

Comment 6

[Matt Woodrow (:mattwoodrow)]

I had a chat with seth about this.

It looks like we're using a spritesheet for these images, and the xul image box has crazy subpixel positioning and sizing.

The combination of these things mean that we end up resizing the image slightly, and sampling from some of the pixels of the next image in the sheet, leaving these grey lines.

The best solution is probably just to change the spritesheet to have slightly larger gaps between images.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> I had a chat with seth about this.
> 
> It looks like we're using a spritesheet for these images, and the xul image
> box has crazy subpixel positioning and sizing.
> 
> The combination of these things mean that we end up resizing the image
> slightly, and sampling from some of the pixels of the next image in the
> sheet, leaving these grey lines.
> 
> The best solution is probably just to change the spritesheet to have
> slightly larger gaps between images.

That doesn't sound right at all. We should be able to specify exactly the area we need. Are you saying we're doing this incorrectly? Or is Gecko not doing what it's being told?

Comment 8

[:Gijs (out for now; he/him)]

Concurring with Dão, we should be able to specify the region and have it "just work". It's not clear to me why the positioning or sizing of the box even matters - we specify the image file and a source region in that image file, and that's what it should use for rendering the background, nothing outside that region (irrespective of the size, antialiasing or otherwise of the box that we happen to use the background image on).

I'd also note that updating all our sprites across platforms and updating all the image regions in the CSS for those sprites would be a considerable task, likely with fallout and regressions, that I wouldn't really want to have to land with less than a week to go until uplift...

Comment 9

[Matt Woodrow (:mattwoodrow)]

Ah, I didn't realize that the xul imagebox element let you explicitly specify a subrect to use. That's nice, and makes it more likely a gecko bug.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Use the source rect

Looks like we basically entirely ignore the source rect that xul is specifying. We use it to create an ClippedImage, and then don't use that.

This seems to work, what do you think seth?

Comment 11

[Matt Woodrow (:mattwoodrow)]

seth, I also notice that ClippedImage won't take advantage of source clipping through Moz2D. Is there a plan to change that?

Comment 12

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8502361 [details] [diff] [review]
Use the source rect

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

Oh wow, facepalm. Thanks for tracking this down.

Comment 13

[Seth Fowler [:seth] [:s2h]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> seth, I also notice that ClippedImage won't take advantage of source
> clipping through Moz2D. Is there a plan to change that?

We definitely should change that. I have some questions before I make the change, so I'll hold off on filing the bug until we can talk and I know exactly what bug to file.

Comment 14

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ab2fdaadd1a

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for reftest failure and mochitest-bc crashes. Please make sure this is green on Try before pushing again. Especially given that this is the weekend before the uplift and tree closures and bustage can be rather disruptive.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f547cf19d104

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2948117&repo=mozilla-inbound
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2947705&repo=mozilla-inbound

Comment 16

[Matt Woodrow (:mattwoodrow)]

Attachment: Use the source rect v2

This version passes try.

Comment 17

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8504472 [details] [diff] [review]
Use the source rect v2

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

This approach is an option if we can't do better, but I have the feeling this can be made cleaner.

Specifically...

::: layout/base/nsLayoutUtils.h
@@ +1599,5 @@
> +   * aImageSourceArea and the original image size.
> +   */
> +  static nsRect GetClippedImageDestination(const nsIntSize& aWholeImageSize,
> +                                           const nsRect& aImageSourceArea,
> +                                           const nsRect& aDestArea);

I'd prefer not to add this. Can't we get the same effect by munging |source| and |imageSize| in |DrawSingleImage|? Both of those variables only exist to serve as arguments for |GetWholeImageDestination|, so it's fine to adjust them as needed to make |GetWholeImageDestination| happy.

Comment 18

[Matt Woodrow (:mattwoodrow)]

Attachment: Use the source rect v3

Ok, how about this then?

Comment 19

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8508475 [details] [diff] [review]
Use the source rect v3

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

Thanks, this looks good!

::: layout/base/nsLayoutUtils.cpp
@@ +5328,5 @@
> +                   imageSize.width*appUnitsPerCSSPixel,
> +                   imageSize.height*appUnitsPerCSSPixel);
> +  nsRect clippedSource = imageRect.Intersect(source);
> +
> +  source -= clippedSource.TopLeft();

You only need to do this clipping if |aSourceArea| is non-null, so you could move this into the |if (aSourceArea)| block and change the value of |imageSize| directly. The common case is that |aSourceArea| is null (I think only XUL uses this parameter?) so it might be nice to move this work out of the main path.

Comment 20

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f831a50b8976

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f831a50b8976

Comment 23

[Luís Miguel [:quicksaver] (not reading bugmail)]

I think this should be uplifted to aurora because of bug 1077997

Comment 24

[Luís Miguel [:quicksaver] (not reading bugmail)]

Matt, can this be uplifted to aurora please? See comment 23

Comment 25

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8508475 [details] [diff] [review]
Use the source rect v3

Approval Request Comment
[Feature/regressing bug #]: Bug 1044702.
[User impact if declined]: Incorrect clipping of some XUL images, visible lines shown at edges.
[Describe test coverage new/current, TBPL]: Tested manually.
[Risks and why]: Low risk, fairly simple patch in the end, only affects XUL.
[String/UUID change made/needed]: None.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/69d6e7ad02ab