Remove Overlay Scrollbar if not scrollable along the axis

Remove Overlay Scrollbar if not scrollable along the axis

Remove Overlay Scrollbar if not scrollable along that axis. If the 
scrollSize along that axis is zero then set scrollbar along that
axis to false. This is applicable to Solid-Color scrollbars in Android
& Overlay scrollbars in the Aura.

BUG=415031
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183445

[MuVen, 2014-09-20 14:17:03]

sataya.m@samsung.com changed reviewers:
+ aelias@chromium.org, danakj@chromium.org

[MuVen, 2014-09-20 14:17:25]

PTAL.

[MuVen, 2014-09-20 14:17:47]

sataya.m@samsung.com changed reviewers:
+ weiliangc@chromium.org

[MuVen, 2014-09-20 14:18:52]

+weiliangc, vivekg

[MuVen, 2014-09-20 14:19:11]

Patchset #2 (id:20001) has been deleted

[MuVen, 2014-09-20 14:19:19]

Patchset #1 (id:1) has been deleted

[MuVen, 2014-09-20 14:25:43]

Patchset #1 (id:40001) has been deleted

[MuVen, 2014-09-22 14:04:20]

PTAL.

[weiliangc, 2014-09-22 21:40:18]

On 2014/09/22 14:04:20, muven wrote:
> PTAL.

This is not a Android only problem.

One potential solution would be in ScrollingCoordinator, when overlay scrollbar
is on and it is not scrollable in that orientation, do not create the overlay
scrollbar.


Also since these code is in Blink, you'll need owners on Blink side.

[MuVen, 2014-09-24 11:23:20]

sataya.m@samsung.com changed reviewers:
+ enne@chromium.org

[MuVen, 2014-09-24 11:34:34]

sataya.m@samsung.com changed reviewers:
- enne@chromium.org

[MuVen, 2014-09-24 12:01:10]

sataya.m@samsung.com changed reviewers:
+ enne@chromium.org

[MuVen, 2014-09-24 12:01:11]

Hi Enne,

PTAL.

Thanks,
MuVen.

[MuVen, 2014-09-24 15:07:29]

sataya.m@samsung.com changed reviewers:
+ vollick@chromium.org
- aelias@chromium.org, danakj@chromium.org, weiliangc@chromium.org

[enne (OOO), 2014-09-24 16:52:03]

Can you write a test for this?

[MuVen, 2014-09-25 13:23:16]

PTAL.

[MuVen, 2014-09-25 13:32:03]

This needs new test or can modify current tests ?

[enne (OOO), 2014-09-25 17:19:34]

Thanks, that test seems reasonable to me.  Can you examine the test failures and
see if those seem expected, and update TestExpectations?

[MuVen, 2014-09-26 11:47:36]

Patchset #7 (id:180001) has been deleted

[MuVen, 2014-09-26 12:04:35]

PTAL.

Thanks,

[enne (OOO), 2014-09-26 17:35:48]

https://codereview.chromium.org/588013002/diff/200001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/588013002/diff/200001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:1344: crbug.com/415031 [ Mac ]
fast/forms/select/listbox-overlay-scrollbar.html [ ImageOnlyFailure ]
Why are you leaving this as a failure?

[MuVen, 2014-09-27 11:36:50]

Patchset #8 (id:220001) has been deleted

[MuVen, 2014-09-27 11:50:43]

Patchset #8 (id:240001) has been deleted

[MuVen, 2014-09-27 11:52:36]

Patchset #8 (id:260001) has been deleted

[MuVen, 2014-09-28 10:40:15]

Patchset #9 (id:300001) has been deleted

[MuVen, 2014-09-28 12:05:55]

Patchset #10 (id:340001) has been deleted

[MuVen, 2014-09-28 12:06:57]

Patchset #10 (id:360001) has been deleted

[MuVen, 2014-09-28 12:07:11]

Patchset #8 (id:280001) has been deleted

[MuVen, 2014-09-29 06:36:33]

Patchset #9 (id:380001) has been deleted

[MuVen, 2014-09-29 08:55:18]

Patchset #9 (id:400001) has been deleted

[MuVen, 2014-09-29 11:08:45]

Patchset #9 (id:420001) has been deleted

[MuVen, 2014-09-29 11:13:29]

Patchset #9 (id:440001) has been deleted

[MuVen, 2014-09-29 11:17:01]

Patchset #9 (id:460001) has been deleted

[MuVen, 2014-09-29 11:21:06]

Patchset #9 (id:480001) has been deleted

[MuVen, 2014-09-29 11:37:09]

Patchset #9 (id:500001) has been deleted

[MuVen, 2014-09-29 11:42:28]

Patchset #9 (id:520001) has been deleted

[MuVen, 2014-09-29 13:09:52]

Patchset #9 (id:540001) has been deleted

[MuVen, 2014-09-29 13:13:11]

On 2014/09/26 17:35:48, enne wrote:
>
https://codereview.chromium.org/588013002/diff/200001/LayoutTests/TestExpecta...
> File LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/588013002/diff/200001/LayoutTests/TestExpecta...
> LayoutTests/TestExpectations:1344: crbug.com/415031 [ Mac ]
> fast/forms/select/listbox-overlay-scrollbar.html [ ImageOnlyFailure ]
> Why are you leaving this as a failure?

Hi Enne,

I tried to upload patch without testexceptions, i see above issues in the
mac_blink_rel.
fast/forms/select/listbox-overlay-scrollbar.html i have modified but the build
bot is not
picking up the modified html.If you see listbox-overlay-scrollbar.html it has
option tag after select tag.
where as in the failure case the modified .html doesnt have option tag. 

Kindly suggest what needs to be done. Thanks,

[MuVen, 2014-09-30 08:59:06]

Patchset #9 (id:560001) has been deleted

[MuVen, 2014-09-30 09:01:13]

Patchset #9 (id:580001) has been deleted

[MuVen, 2014-09-30 09:02:17]

Patchset #9 (id:600001) has been deleted

[MuVen, 2014-09-30 12:49:48]

https://codereview.chromium.org/588013002/diff/200001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/588013002/diff/200001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:1344: crbug.com/415031 [ Mac ]
fast/forms/select/listbox-overlay-scrollbar.html [ ImageOnlyFailure ]
On 2014/09/26 17:35:48, enne wrote:
> Why are you leaving this as a failure?

Enne, Can we make listbox-overlay-scrollbar png test instead of making it ref
tests. Even after modifying the html i see it breaks in the mac. With your
consent i wanna remove listbox-overlay-scrollbar-expected.html. Please mention
your views

[enne (OOO), 2014-10-01 18:13:11]

On 2014/09/30 at 12:49:48, sataya.m wrote:
>
https://codereview.chromium.org/588013002/diff/200001/LayoutTests/TestExpecta...
> File LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/588013002/diff/200001/LayoutTests/TestExpecta...
> LayoutTests/TestExpectations:1344: crbug.com/415031 [ Mac ]
fast/forms/select/listbox-overlay-scrollbar.html [ ImageOnlyFailure ]
> On 2014/09/26 17:35:48, enne wrote:
> > Why are you leaving this as a failure?
> 
> Enne, Can we make listbox-overlay-scrollbar png test instead of making it ref
tests. Even after modifying the html i see it breaks in the mac. With your
consent i wanna remove listbox-overlay-scrollbar-expected.html. Please mention
your views

I would strongly prefer to keep ref tests.  Can you adjust the html to also work
on the mac?

[MuVen, 2014-10-01 23:48:19]

On 2014/10/01 18:13:11, enne wrote:
> On 2014/09/30 at 12:49:48, sataya.m wrote:
> >
>
https://codereview.chromium.org/588013002/diff/200001/LayoutTests/TestExpecta...
> > File LayoutTests/TestExpectations (right):
> > 
> >
>
https://codereview.chromium.org/588013002/diff/200001/LayoutTests/TestExpecta...
> > LayoutTests/TestExpectations:1344: crbug.com/415031 [ Mac ]
> fast/forms/select/listbox-overlay-scrollbar.html [ ImageOnlyFailure ]
> > On 2014/09/26 17:35:48, enne wrote:
> > > Why are you leaving this as a failure?
> > 
> > Enne, Can we make listbox-overlay-scrollbar png test instead of making it
ref
> tests. Even after modifying the html i see it breaks in the mac. With your
> consent i wanna remove listbox-overlay-scrollbar-expected.html. Please mention
> your views
> 
> I would strongly prefer to keep ref tests.  Can you adjust the html to also
work
> on the mac?

Hi Enne,

I tried to upload patch without testexceptions, i see above issues in the
mac_blink_rel.
fast/forms/select/listbox-overlay-scrollbar.html i have modified but the build
bot is not
picking up the modified html.If you see listbox-overlay-scrollbar.html it has
option tag after select tag.
where as in the failure case the modified .html doesnt have option tag.

[enne (OOO), 2014-10-02 15:25:17]

I don't understand what you mean by "the build bot is not picking up the
modified html".

[MuVen, 2014-10-02 17:07:01]

On 2014/10/02 15:25:17, enne wrote:
> I don't understand what you mean by "the build bot is not picking up the
> modified html".

yes

[enne (OOO), 2014-10-02 18:19:15]

On 2014/10/02 at 17:07:01, sataya.m wrote:
> On 2014/10/02 15:25:17, enne wrote:
> > I don't understand what you mean by "the build bot is not picking up the
> > modified html".
> 
> yes

Sorry, I still don't understand.  I was looking for a more detailed explanation
of what you meant by "the build bot is not picking up the modified html".

[MuVen, 2014-10-03 02:56:02]

Hi enne,


https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/25...

is the result of mac_blink_rel bot which i have triggered with some 
changes in listbox-overlay-scrollbar.html which you can see in the 
patch(removing height in the .sel, specifying size of select tag to 5,
adding option tag with in the select tag).But when i click on the 
link fast/forms/select/listbox-overlay-scrollbar.html which is showed
as failed test case in the above mentioned link it is showing the html
without my changes which is causing failure of test case i suppose.

[MuVen, 2014-10-03 09:45:11]

Patchset #11 (id:660001) has been deleted

[MuVen, 2014-10-03 11:41:32]

Patchset #12 (id:700001) has been deleted

[MuVen, 2014-10-03 11:41:46]

Patchset #11 (id:680001) has been deleted

[MuVen, 2014-10-03 11:42:22]

Patchset #10 (id:640001) has been deleted

[MuVen, 2014-10-03 12:59:26]

Hi enne,

i have upload patchset 10 and this also in which mac bot has failed.

When you open the link layout test results @
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/25...

and check the html page source i see that bot picks up only
expected file from patch and original file from server.
Seeing this i assume that i need to modify only expected file.
(correct me if im wrong).

[weiliangc, 2014-10-03 14:36:50]

On 2014/10/03 12:59:26, muven wrote:
> Hi enne,
> 
> i have upload patchset 10 and this also in which mac bot has failed.
> 
> When you open the link layout test results @
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/25...
> 
> and check the html page source i see that bot picks up only
> expected file from patch and original file from server.
> Seeing this i assume that i need to modify only expected file.
> (correct me if im wrong).

Looks like it's failing with and without patch. (I believe the fail at top is
with patch, fail at bottom is without patch).

You can try rebase your patch?

[MuVen, 2014-10-03 16:04:22]

On 2014/10/03 14:36:50, weiliangc wrote:
> On 2014/10/03 12:59:26, muven wrote:
> > Hi enne,
> > 
> > i have upload patchset 10 and this also in which mac bot has failed.
> > 
> > When you open the link layout test results @
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/25...
> > 
> > and check the html page source i see that bot picks up only
> > expected file from patch and original file from server.
> > Seeing this i assume that i need to modify only expected file.
> > (correct me if im wrong).
> 
> Looks like it's failing with and without patch. (I believe the fail at top is
> with patch, fail at bottom is without patch).
> 
> You can try rebase your patch?

i did by adding it in test-expectations(i have deleted that patch 
here as i saw the same error).

[MuVen, 2014-10-03 16:06:57]

On 2014/10/03 16:04:22, muven wrote:
> On 2014/10/03 14:36:50, weiliangc wrote:
> > On 2014/10/03 12:59:26, muven wrote:
> > > Hi enne,
> > > 
> > > i have upload patchset 10 and this also in which mac bot has failed.
> > > 
> > > When you open the link layout test results @
> > >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/25...
> > > 
> > > and check the html page source i see that bot picks up only
> > > expected file from patch and original file from server.
> > > Seeing this i assume that i need to modify only expected file.
> > > (correct me if im wrong).
> > 
> > Looks like it's failing with and without patch. (I believe the fail at top
is
> > with patch, fail at bottom is without patch).
> > 
> > You can try rebase your patch?
> 
> i did by adding it in test-expectations(i have deleted that patch 
> here as i saw the same error).

May be shall i need to do the same on the latest ?
by adding LayoutTests/fast/forms/select/listbox-overlay-scrollbar.html [
NeedsReb
      aseline ] ?

[MuVen, 2014-10-04 05:58:39]

Patchset #11 (id:740001) has been deleted

[MuVen, 2014-10-04 07:19:16]

Patchset #11 (id:760001) has been deleted

[MuVen, 2014-10-04 07:25:13]

Patchset #11 (id:780001) has been deleted

[MuVen, 2014-10-04 07:32:07]

Patchset #11 (id:800001) has been deleted

[MuVen, 2014-10-04 07:51:56]

Patchset #12 (id:840001) has been deleted

[MuVen, 2014-10-04 07:59:23]

Patchset #12 (id:860001) has been deleted

[MuVen, 2014-10-04 08:01:07]

Patchset #12 (id:880001) has been deleted

[MuVen, 2014-10-04 10:31:48]

Patchset #14 (id:940001) has been deleted

[MuVen, 2014-10-05 10:58:14]

Patchset #18 (id:1040001) has been deleted

[MuVen, 2014-10-06 03:54:32]

Patchset #20 (id:1100001) has been deleted

[MuVen, 2014-10-06 03:54:44]

Patchset #19 (id:1080001) has been deleted

[MuVen, 2014-10-06 03:54:56]

Patchset #18 (id:1060001) has been deleted

[MuVen, 2014-10-06 03:55:08]

Patchset #17 (id:1020001) has been deleted

[MuVen, 2014-10-06 03:55:25]

Patchset #16 (id:1000001) has been deleted

[MuVen, 2014-10-06 03:55:43]

Patchset #15 (id:980001) has been deleted

[MuVen, 2014-10-06 03:56:00]

Patchset #14 (id:960001) has been deleted

[MuVen, 2014-10-06 03:57:34]

Patchset #13 (id:920001) has been deleted

[MuVen, 2014-10-06 03:57:46]

Patchset #12 (id:900001) has been deleted

[MuVen, 2014-10-06 03:57:57]

Patchset #11 (id:820001) has been deleted

[MuVen, 2014-10-06 03:58:10]

Patchset #10 (id:720001) has been deleted

[MuVen, 2014-10-06 03:58:20]

Patchset #9 (id:620001) has been deleted

[MuVen, 2014-10-06 03:58:31]

Patchset #8 (id:320001) has been deleted

[MuVen, 2014-10-06 13:34:27]

Hi Enne,

I have fixed all the issues. PTAL.

Thanks,
Muven.

https://codereview.chromium.org/588013002/diff/200001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/588013002/diff/200001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:1344: crbug.com/415031 [ Mac ]
fast/forms/select/listbox-overlay-scrollbar.html [ ImageOnlyFailure ]
Modified list-box-overlay-scrollbar.html and its working on mac, linux, win
platforms.Removing this test exception.

[MuVen, 2014-10-07 17:29:52]

On 2014/10/06 13:34:27, muven wrote:
> Hi Enne,
> 
> I have fixed all the issues. PTAL.
> 
> Thanks,
> Muven.
> 
>
https://codereview.chromium.org/588013002/diff/200001/LayoutTests/TestExpecta...
> File LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/588013002/diff/200001/LayoutTests/TestExpecta...
> LayoutTests/TestExpectations:1344: crbug.com/415031 [ Mac ]
> fast/forms/select/listbox-overlay-scrollbar.html [ ImageOnlyFailure ]
> Modified list-box-overlay-scrollbar.html and its working on mac, linux, win
> platforms.Removing this test exception.

friendly ping !!!

[enne (OOO), 2014-10-07 18:57:22]

https://codereview.chromium.org/588013002/diff/1120001/LayoutTests/fast/forms...
File LayoutTests/fast/forms/select/listbox-overlay-scrollbar.html (right):

https://codereview.chromium.org/588013002/diff/1120001/LayoutTests/fast/forms...
LayoutTests/fast/forms/select/listbox-overlay-scrollbar.html:33:
<option>foo1</option>
Can you tell me why you needed to add all these options? Does this test have
overflow?  (If so, is there a test that verifies that not having overflow means
no scrollbars?)

[MuVen, 2014-10-07 23:45:42]

https://codereview.chromium.org/588013002/diff/1120001/LayoutTests/fast/forms...
File LayoutTests/fast/forms/select/listbox-overlay-scrollbar.html (right):

https://codereview.chromium.org/588013002/diff/1120001/LayoutTests/fast/forms...
LayoutTests/fast/forms/select/listbox-overlay-scrollbar.html:33:
<option>foo1</option>
This test is for to check overlay scrollbar with in the list box.
Without adding these option tag i was not able to pass this test 
in the mac platform(as i see mac displays the scrollbar upon
some content with in the listbox).There is not test for not having
scrollbars.

[MuVen, 2014-10-08 00:41:37]

sorry.to explain in detail this test case is to ensure that listbox
also renders the overlay scrollbar. As per out current patch if the
scrollsize horizontally or vertically is zero this will set the 
scrollbars to false.So due to this patch the overlay scrollbars of
listbox were removed. To show the scrollbar with in the listbox has
made me to add option tag with in the select tag. I have tried to
add a 5 empty <option></option> tags to display scrollbar vertically.
But that was working only in linux and win. Due to which i have added
<option>fool</option> tag so that in mac also it creates a vertical
scrollbar upon overflow.May be i shall add a one more div to show 
empty size in the listbox shall not display overlay scrollbars.

[enne (OOO), 2014-10-08 00:52:28]

Thanks, that makes a lot of sense to have the listbox test continue to test what
it was testing before.

I think I'd really like to see a similar layout test where overlay scrollbar
*don't* appear.  Maybe this could be a new test, or maybe you could add it to
overlay-scrollbars.html, in order to verify the logic you added in this patch
for when these scrollbars don't appear.

[MuVen, 2014-10-08 14:07:26]

I have uploaded listbox-nooverlay-scrollbar.html which disappears the 
overlay scrollbar on empty listbox size.

[enne (OOO), 2014-10-08 20:41:49]

lgtm

Thanks for all the test investigations and changes.

[enne (OOO), 2014-10-08 20:42:11]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-10-08 20:43:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/588013002/1140001

[commit-bot: I haz the power, 2014-10-09 02:35:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-09 02:35:45]

Exceeded time limit waiting for builds to trigger.

[MuVen, 2014-10-09 05:31:25]

The CQ bit was checked by sataya.m@samsung.com

[commit-bot: I haz the power, 2014-10-09 05:32:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/588013002/1140001

[commit-bot: I haz the power, 2014-10-09 06:49:20]

Committed patchset #9 (id:1140001) as 183445

[apavlov, 2014-10-10 08:19:49]

A revert of this CL (patchset #9 id:1140001) has been created in
https://codereview.chromium.org/643033002/ by apavlov@chromium.org.

The reason for reverting is: This has tentatively resulted in multiple
webkit_unit_tests crashes on android_dbg_tests_recipe, blocking a Blink roll..

[MuVen, 2014-10-13 06:24:57]

Patchset #10 (id:1280001) has been deleted