1062204 - Pictures can't be scrolled in WebIDE and break scroll functionality afterwards

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Petruta Horea [:phorea], Desktop QA]

Attachment: PngView.png

Reproducible on:
latest Nightly 35.0a1 - 20140902121319
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
latest Aurora 34.0a2 - 20140902121245
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Firefox 33 beta 1 - 20140902214533 (after enabling WebIDE)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0

Steps to reproduce:
1. Open WebIDE (Shift+F8) and add an app
2. Verify that the left and the main area can be scrolled while opening different file types
3. Open a img/png file that doesn't fit the view (is larger)
4. Scroll other file types

Actual results:
3. The img/png files cannot be scrolled
4. Horizontal and vertical scroll is broken after viewing 

Expected results:
All files can be scrolled; scroll functionality is not broken

Note: 1. The issue reproduces also under Linux 12.10 32-bit and Mac OSX 10.8.5
2. The issue reproduces before bug 1027537 was fixed

Comment 1

[Brian Grinstead [:bgrins]]

I am seeing the inability to scroll an image in OSX and that the scrollbar disappears on the next file opened (although I can still scroll with the mouse wheel, and it comes back after resizing).

However, I'm not seeing the inability to scroll the files area after opening the image.  There is definitely something going on when opening an image bigger than the content area though, hopefully fixing this will fix all of the scrollbar issues.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Jen, this looks like a good bug to check out to learn about the WebIDE code base.

Let me know if you have any questions about it.

Comment 3

[Jen Fong-Adwent [:ednapiranha]]

Attachment: bug1062204.patch

Let me know if you want me to add a small test to this fix - thanks!

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8490313 [details] [diff] [review]
bug1062204.patch

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

Ah, okay, I see what you meant over IRC now.  Yeah, vbox is very general and used for many purposes, so we should probably use a more specific selector here.

I believe the specific vbox you want is the one that is part of the |Shell| object[1].  I'd suggest adding a class to that vbox and using this as the selector here.

I don't feel like a test is needed here since it's a CSS-only change.

[1]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/projecteditor/lib/shells.js#31

Comment 5

[Jen Fong-Adwent [:ednapiranha]]

Attachment: bug1062204.patch

I found an existing id to attach to within that deck. Let me know if that's a safe place to assign the rule.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8490390 [details] [diff] [review]
bug1062204.patch

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

Unfortunately, that's not quite what we want.

If you open several different images, they each get their own vbox inside the deck, and the elements are left in the DOM even when not shown.

With this patch, if you have previously opened both a tall image (that needs scrollbars) and a short image (that does not), then even the short image which might fit within the current viewport is given scrollbars as if it were the size of the large image.  This problem goes away if you target the specific vboxs instead.

Comment 7

[Jen Fong-Adwent [:ednapiranha]]

Attachment: bug1062204.patch

Maybe this one? :)

Comment 8

[Paul Rouget [:paul]]

(a note about your commit message: add r=jryans, and say what you do in the patch, don't use the title of the bug: "Bug 1062204 - make pictures scrollable. r=jryans")

Comment 9

[Jen Fong-Adwent [:ednapiranha]]

Attachment: bug1062204.patch

Updating commit msg

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8490440 [details] [diff] [review]
bug1062204.patch

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

Cool, seems to work well to me! :)

The next step is to push this to Try to run tests.

Do you have access to Try?

If you do, please push it using a syntax such as "try: -b do -p all -u xpcshell,mochitest-dt,mochitest-o -t none" to run XPCShell, Browser Mochitests, and Chrome Mochitests.

If you do not, I'll push it this time, but you should also request level 1 commit access[1] in the mean time.

[1]: https://www.mozilla.org/hacking/commit-access-policy/

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cbd2eea1a8d4

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Okay, the Try run looks good (enough...).

Once you've got a green Try run and you want to land your work, you set "checkin-needed" on the bug's keywords field, so feel free to do so!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/18ac09aeaf6c

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/18ac09aeaf6c