Issue 4427 - Anchor tags hidden behind header on acceptableads.com

[juliandoucette, 2017-01-24 23:37:15]

Added Thomas to reviewers.

[juliandoucette, 2017-01-24 23:47:53]

On 2017/01/24 23:37:15, juliandoucette wrote:
> Added Thomas to reviewers.

Updated description.

> Should be a quick one.
> 
> See http://stackoverflow.com/a/13067009 (I only changed code style)
> 
> I think that this is a pretty good solution because it doesn't rely on hacky
CSS. 
> 
> Let me know what you think of the solution first. If you like it, then we will
fix NITs and consider moving it to website-defaults.

[Thomas Greiner, 2017-01-25 11:11:18]

Thanks for investigating potential solutions but this approach is quite a hack.
Reading up on it myself it seems like simply adding a padding before each target
would be sufficient if we were only targeting headlines - not really compatible
with footnotes though so not a good solution either.

One option that I could think of is that we could ask whether it is even
necessary for the header to be sticky. Especially since the entire navigation is
now in the sidebar, the header on acceptableads.com mostly wastes space (and
hides precious content beneath it). Even if we think that the breadcrumbs are
important enough to always be visible, why not only make those sticky?

Another option is that we could make only the content container be scrollable:

e.g.

.outer
{
  position: absolute;
  top: 96px;
  bottom: 0;
  height: auto;
  padding-top: 0;
  overflow: auto;
}

What do you think?

[juliandoucette, 2017-01-25 12:55:39]

> Thanks for investigating potential solutions but this approach is quite a
hack.

Why?

> Reading up on it myself it seems like simply adding a padding before each
target
> would be sufficient if we were only targeting headlines - not really
compatible
> with footnotes though so not a good solution either.

Agreed. That would also force us to apply the same amount of padding (or height)
as the height of the fixed header or use negative values to counteract.

> 
> One option that I could think of is that we could ask whether it is even
> necessary for the header to be sticky. Especially since the entire navigation
is
> now in the sidebar, the header on http://acceptableads.com mostly wastes space
(and
> hides precious content beneath it). Even if we think that the breadcrumbs are
> important enough to always be visible, why not only make those sticky?

Agreed.

I think we should consult product about this (static vs sticky header).

I'm skeptical about whether we can change their mind about mobile though.

> Another option is that we could make only the content container be scrollable:
> 
> e.g.
> 
> .outer
> {
>   position: absolute;
>   top: 96px;
>   bottom: 0;
>   height: auto;
>   padding-top: 0;
>   overflow: auto;
> }
> 
> What do you think?

I like this solution. I'll complete it in my next patchset.

[juliandoucette, 2017-01-25 13:13:25]

> I like this solution. I'll complete it in my next patchset.

Done. See latest patchset.

[Thomas Greiner, 2017-01-31 15:41:43]

On 2017/01/25 12:55:39, juliandoucette wrote:
> > Thanks for investigating potential solutions but this approach is quite a
> hack.
> 
> Why?

I didn't consider it to be important enough to mention but here are some
reasons:
- Accessibility: Only works if JavaScript is enabled.
- Maintenance: The less code we have to maintain the better.
- Performance/reliability: Implements custom behavior instead of relying on
native behaviors and low-level implementations.

> > One option that I could think of is that we could ask whether it is even
> > necessary for the header to be sticky. Especially since the entire
navigation
> is
> > now in the sidebar, the header on http://acceptableads.com mostly wastes
space
> (and
> > hides precious content beneath it). Even if we think that the breadcrumbs
are
> > important enough to always be visible, why not only make those sticky?
> 
> Agreed.
> 
> I think we should consult product about this (static vs sticky header).
> 
> I'm skeptical about whether we can change their mind about mobile though.

I'm also sceptical about that. :D But so far they've been quite open to
feedback.

> > Another option is that we could make only the content container be
scrollable:
> > 
> > e.g.
> > 
> > .outer
> > {
> >   position: absolute;
> >   top: 96px;
> >   bottom: 0;
> >   height: auto;
> >   padding-top: 0;
> >   overflow: auto;
> > }
> > 
> > What do you think?
> 
> I like this solution. I'll complete it in my next patchset.
> ...
> Done. See latest patchset.

LGTM

[aaron, 2017-02-07 08:46:25]

On 2017/01/31 15:41:43, Thomas Greiner wrote:
> On 2017/01/25 12:55:39, juliandoucette wrote:
> > > Thanks for investigating potential solutions but this approach is quite a
> > hack.
> > 
> > Why?
> 
> I didn't consider it to be important enough to mention but here are some
> reasons:
> - Accessibility: Only works if JavaScript is enabled.
> - Maintenance: The less code we have to maintain the better.
> - Performance/reliability: Implements custom behavior instead of relying on
> native behaviors and low-level implementations.
> 
> > > One option that I could think of is that we could ask whether it is even
> > > necessary for the header to be sticky. Especially since the entire
> navigation
> > is
> > > now in the sidebar, the header on http://acceptableads.com mostly wastes
> space
> > (and
> > > hides precious content beneath it). Even if we think that the breadcrumbs
> are
> > > important enough to always be visible, why not only make those sticky?
> > 
> > Agreed.
> > 
> > I think we should consult product about this (static vs sticky header).
> > 
> > I'm skeptical about whether we can change their mind about mobile though.
> 
> I'm also sceptical about that. :D But so far they've been quite open to
> feedback.
> 
> > > Another option is that we could make only the content container be
> scrollable:
> > > 
> > > e.g.
> > > 
> > > .outer
> > > {
> > >   position: absolute;
> > >   top: 96px;
> > >   bottom: 0;
> > >   height: auto;
> > >   padding-top: 0;
> > >   overflow: auto;
> > > }
> > > 
> > > What do you think?
> > 
> > I like this solution. I'll complete it in my next patchset.
> > ...
> > Done. See latest patchset.
> 
> LGTM

Jeen and I agree on the following changes:

For web: Make the page header static.

For mobile: Make the page header static on scroll down, but slide in and remain
when scrolling up.

Thanks!

[aaron, 2017-02-07 08:50:18]

On 2017/02/07 08:46:25, aaron wrote:
> On 2017/01/31 15:41:43, Thomas Greiner wrote:
> > On 2017/01/25 12:55:39, juliandoucette wrote:
> > > > Thanks for investigating potential solutions but this approach is quite
a
> > > hack.
> > > 
> > > Why?
> > 
> > I didn't consider it to be important enough to mention but here are some
> > reasons:
> > - Accessibility: Only works if JavaScript is enabled.
> > - Maintenance: The less code we have to maintain the better.
> > - Performance/reliability: Implements custom behavior instead of relying on
> > native behaviors and low-level implementations.
> > 
> > > > One option that I could think of is that we could ask whether it is even
> > > > necessary for the header to be sticky. Especially since the entire
> > navigation
> > > is
> > > > now in the sidebar, the header on http://acceptableads.com mostly wastes
> > space
> > > (and
> > > > hides precious content beneath it). Even if we think that the
breadcrumbs
> > are
> > > > important enough to always be visible, why not only make those sticky?
> > > 
> > > Agreed.
> > > 
> > > I think we should consult product about this (static vs sticky header).
> > > 
> > > I'm skeptical about whether we can change their mind about mobile though.
> > 
> > I'm also sceptical about that. :D But so far they've been quite open to
> > feedback.
> > 
> > > > Another option is that we could make only the content container be
> > scrollable:
> > > > 
> > > > e.g.
> > > > 
> > > > .outer
> > > > {
> > > >   position: absolute;
> > > >   top: 96px;
> > > >   bottom: 0;
> > > >   height: auto;
> > > >   padding-top: 0;
> > > >   overflow: auto;
> > > > }
> > > > 
> > > > What do you think?
> > > 
> > > I like this solution. I'll complete it in my next patchset.
> > > ...
> > > Done. See latest patchset.
> > 
> > LGTM
> 
> Jeen and I agree on the following changes:
> 
> For web: Make the page header static.
> 
> For mobile: Make the page header static on scroll down, but slide in and
remain
> when scrolling up.
> 
> Thanks!

Here's the ticket for reference:
https://issues.adblockplus.org/ticket/4868#ticket

[juliandoucette, 2017-02-23 12:51:44]

> > Jeen and I agree on the following changes:
> > 
> > For web: Make the page header static.
> > 
> > For mobile: Make the page header static on scroll down, but slide in and
> remain
> > when scrolling up.
> > 
> > Thanks!
> 
> Here's the ticket for reference:
> https://issues.adblockplus.org/ticket/4868#ticket

Thanks Aaron!

We will address the issue that you created separately because this solution
fixes the problem outlined in #4427 effectively on all platforms and the
solution to #4868 will take more time to complete.

[Robert Raceanu, 2017-03-01 01:38:22]

On 2017/02/23 12:51:44, juliandoucette wrote:
> > > Jeen and I agree on the following changes:
> > > 
> > > For web: Make the page header static.
> > > 
> > > For mobile: Make the page header static on scroll down, but slide in and
> > remain
> > > when scrolling up.
> > > 
> > > Thanks!
> > 
> > Here's the ticket for reference:
> > https://issues.adblockplus.org/ticket/4868#ticket
> 
> Thanks Aaron!
> 
> We will address the issue that you created separately because this solution
> fixes the problem outlined in #4427 effectively on all platforms and the
> solution to #4868 will take more time to complete.

LGTM - Verified on Chrome/Firefox/IE on Windows 7,8 and 10, Chrome/Firefox and
Safari on Mac and Chrome/Firefox/ABB on mobile

[Robert Raceanu, 2017-03-01 01:40:30]

On 2017/03/01 01:38:22, Robert Raceanu wrote:
> On 2017/02/23 12:51:44, juliandoucette wrote:
> > > > Jeen and I agree on the following changes:
> > > > 
> > > > For web: Make the page header static.
> > > > 
> > > > For mobile: Make the page header static on scroll down, but slide in and
> > > remain
> > > > when scrolling up.
> > > > 
> > > > Thanks!
> > > 
> > > Here's the ticket for reference:
> > > https://issues.adblockplus.org/ticket/4868#ticket
> > 
> > Thanks Aaron!
> > 
> > We will address the issue that you created separately because this solution
> > fixes the problem outlined in #4427 effectively on all platforms and the
> > solution to #4868 will take more time to complete.
> 
> LGTM - Verified on Chrome/Firefox/IE on Windows 7,8 and 10, Chrome/Firefox and
> Safari on Mac and Chrome/Firefox/ABB on Android and iOS

[juliandoucette, 2017-03-31 12:40:51]

I've verified that this patch still works. Do you have any objections to pushing
saroyanm?

[saroyanm, 2017-03-31 13:58:20]

On 2017/03/31 12:40:51, juliandoucette wrote:
> I've verified that this patch still works. Do you have any objections to
pushing
> saroyanm?

LGTM

[juliandoucette, 2017-04-03 19:50:41]

> LGTM

https://hg.adblockplus.org/web.acceptableads.com/rev/95a74f052736