Last Comment Bug 778174 - [responsive mode] add a vertical resizer
: [responsive mode] add a vertical resizer
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Responsive Design Mode (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Firefox 25
Assigned To: André Miranda
: developer.tools
Mentors:
Depends on:
Blocks: 749628
  Show dependency treegraph
 
Reported: 2012-07-27 09:08 PDT by Paul Rouget [:paul]
Modified: 2013-08-16 06:38 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch that solves the bug (13.95 KB, patch)
2013-07-08 19:48 PDT, André Miranda
no flags Details | Diff | Splinter Review
Resource necessary for the patch (256 bytes, image/png)
2013-07-08 19:51 PDT, André Miranda
no flags Details
Image vs CSS Transform comparison (63.28 KB, image/png)
2013-07-09 16:26 PDT, André Miranda
no flags Details
Testing blur of transform-rotate. (1.39 KB, application/octet-stream)
2013-07-10 06:17 PDT, André Miranda
no flags Details
Rendering bug (38.03 KB, image/png)
2013-07-12 09:20 PDT, André Miranda
no flags Details
Patch css-rotate (17.37 KB, text/plain)
2013-07-17 20:32 PDT, André Miranda
no flags Details
Patch new-image (16.28 KB, patch)
2013-07-17 20:34 PDT, André Miranda
paul: review+
Details | Diff | Splinter Review
patch-fix-rotate (997 bytes, patch)
2013-08-14 20:43 PDT, André Miranda
no flags Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2012-07-27 09:08:02 PDT

    
Comment 1 Paul Rouget [:paul] 2012-08-27 03:13:10 PDT
Hubert, could you take a look at this?
Comment 2 Paul Rouget [:paul] 2012-12-03 10:27:52 PST
Hubert, do you still want to work on that?
Comment 3 hsablonniere 2012-12-03 10:35:33 PST
Yes, I would love to but I'm a bit overwhelmed with my teaching. I know this feature is important. What's the timeframe for FF 20?

I already had a look at how it could be implemented. I think a weekend would do the trick (including tests...) but I won't be able to do it before the 21 dec.

What do we do?
Comment 4 Paul Rouget [:paul] 2012-12-03 10:46:56 PST
No hurry. No deadline.

> What do we do?

We wait until you have time :)
Comment 5 hsablonniere 2012-12-03 10:48:26 PST
Why did I ask? Mozilla I love you!!
Comment 6 André Miranda 2013-07-08 19:48:39 PDT
Created attachment 772454 [details] [diff] [review]
Patch that solves the bug

Sorry Hubert, I couldn't wait :D
I just picked this for my first bugfix as it seemed pretty easy.
As I've said, it's my first bugfix here, please evaluate the patch and tell me what I have done wrong :D
PS: I don't know how to send a binary file(png) along with the patch, so in order to this patch work correctly, the file browser/themes/<platform>/devtools/responsive-vertical-resizer.png should be cloned, rotated(90°) and renamed as responsive-horizontal-resizer.png. Sorry for the inconvenience :(
Comment 7 André Miranda 2013-07-08 19:51:04 PDT
Created attachment 772456 [details]
Resource necessary for the patch

Just in case someone needs the png file.
Comment 8 hsablonniere 2013-07-09 00:14:08 PDT
Hey André, no worries. I've been pretty busy these days. Thx for your submission...
Comment 9 Paul Rouget [:paul] 2013-07-09 00:36:51 PDT
Comment on attachment 772454 [details] [diff] [review]
Patch that solves the bug

André, thanks a lot for your help.

To add a file (binary or not), just do "hg add path/to/the/file".

Instead of creating a new file though, I'd suggest that you use responsive-vertical-resizer.png and you apply a CSS transform to it (transform:rotate(90deg)). You might want want to rename this file to "responsive-bar-resizer.png".

You use translate(12,12). You want to use translate(-12,12) (to align horizontally the image).

In the function `onDrag`, you should do: `if (this.ignoreX) deltaX = 0`.
We don't want to change the width with this resizer is used.
Comment 10 André Miranda 2013-07-09 16:26:48 PDT
Created attachment 772963 [details]
Image vs CSS Transform comparison

Paul, thanks for the directions.
I did as you said, but the results of css transform are not nice.
I think it's a good idea to reuse a resource, but in this case there's a deterioration of the image quality.
Check the attachment.
Comment 11 Paul Rouget [:paul] 2013-07-10 05:48:24 PDT
Comment on attachment 772963 [details]
Image vs CSS Transform comparison

This is weird. And it should not happen. Can I ask you to try to reproduce this bug within a web page?
Comment 12 André Miranda 2013-07-10 06:17:29 PDT
Created attachment 773252 [details]
Testing blur of transform-rotate.

I've setup a small test, please take a look.
Same behavior under Chromium 26.
Comment 13 Paul Rouget [:paul] 2013-07-10 09:49:17 PDT
Interesting. What happens if you add `image-rendering: -moz-crisp-edges`?
Comment 14 Girish Sharma [:Optimizer] 2013-07-10 09:55:12 PDT
The issue is because of odd number of pixels in width. And yes, `image-rendering: -moz-crisp-edges` fixes the issue.
Comment 15 André Miranda 2013-07-10 09:56:59 PDT
Great, now it looks nice.
What do you think about "rotate(90deg) translate(20px, -12px)",  these values were based on trial and error, do think there's a better way to do it?
Latter I'll prepare another patch.
Once again thanks.
Comment 16 Paul Rouget [:paul] 2013-07-11 00:56:08 PDT
You should use `translate(12px, -12px) ` to match what we do for the other resizer. But start with translate then rotate `translate(12,-12) rotate(90deg)`. I think this will work.
Comment 17 André Miranda 2013-07-12 09:20:42 PDT
Created attachment 774707 [details]
Rendering bug

Once again something strange happened. I've followed your instructions and `translate(12,-12) rotate(90deg)` doesn't work as it should(see attachment). Then I tried `rotate(90deg) translate(20px, 5px)` and worked just fine under Linux. Then I hacked the onmi.ja under Windows and while dragging the bar it becomes weird(see attachment). At work's machine, I also hacked the onmi.ja, but it seems ok.
Any guess?

My box: Archlinux | Windows 64-bit Ultimate - Core2Duo + NVidia GTX460
Work's box: Windows 64-bit Professional - Phenom II + Radeon HD 4250
Comment 18 Paul Rouget [:paul] 2013-07-15 02:51:54 PDT
Can you attach the patch?
Comment 19 André Miranda 2013-07-17 20:32:46 PDT
Created attachment 777558 [details]
Patch css-rotate

This patch uses the css transform rotate. Works, but only on my PC has shown some bugs.
Comment 20 André Miranda 2013-07-17 20:34:47 PDT
Created attachment 777560 [details] [diff] [review]
Patch new-image

This patch creates new rotated image. Works fine on every PC I've tested.
Comment 21 Paul Rouget [:paul] 2013-07-30 23:45:05 PDT
Comment on attachment 777560 [details] [diff] [review]
Patch new-image

Let's go for this approach.

Thank you André.
Comment 22 Paul Rouget [:paul] 2013-07-30 23:47:08 PDT
https://tbpl.mozilla.org/?tree=Try&rev=964d12259d90
Comment 23 Paul Rouget [:paul] 2013-08-01 00:41:31 PDT
This orange (bug 866671) is triggered on 4 platforms with this patch. Let's try with a more recent tree: https://tbpl.mozilla.org/?tree=Try&rev=c3646bda31c8
Comment 24 Paul Rouget [:paul] 2013-08-01 06:18:52 PDT
That's better.
Comment 26 Ryan VanderMeulen [:RyanVM] 2013-08-01 17:30:33 PDT
https://hg.mozilla.org/mozilla-central/rev/648ab92d0f63
Comment 27 Paul Rouget [:paul] 2013-08-02 01:21:49 PDT
s/12px/-12px/ https://hg.mozilla.org/integration/fx-team/rev/eb7ff2beda97
Comment 28 Ryan VanderMeulen [:RyanVM] 2013-08-02 16:58:45 PDT
https://hg.mozilla.org/mozilla-central/rev/eb7ff2beda97
Comment 29 André Miranda 2013-08-14 20:43:47 PDT
Created attachment 790619 [details] [diff] [review]
patch-fix-rotate

While testing the nightly, I noticed a problem when resizing with the new resizer and then rotate, it causes the resizer to move to an incorrect position.
Patch attached.
Comment 30 Paul Rouget [:paul] 2013-08-16 05:59:52 PDT
(In reply to André Miranda from comment #29)
> Created attachment 790619 [details] [diff] [review]
> patch-fix-rotate
> 
> While testing the nightly, I noticed a problem when resizing with the new
> resizer and then rotate, it causes the resizer to move to an incorrect
> position.
> Patch attached.

Thank you!
Can I ask you to file a new bug and attach this patch?
Comment 31 André Miranda 2013-08-16 06:38:56 PDT
Done: bug 906025

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