Issue 2356 - Implemented tooltip functionality

Also part of this review:
- Fixed: Long section titles caused lines to overflow
- Fixed: "read more" links were not aligned in the top-right corner of a section

Note that we need to check and update the options page texts at some point anyway. Therefore the tooltip texts should not be considered final yet.

[saroyanm, 2015-07-22 18:25:39]

https://codereview.adblockplus.org/29322581/diff/29322582/options.html
File options.html (right):

https://codereview.adblockplus.org/29322581/diff/29322582/options.html#newcode80
options.html:80: <span class="tooltip">
I think we shouldn't put div inside span, until we really need it, actually
can't find even case why we would need it.

https://codereview.adblockplus.org/29322581/diff/29322582/options.html#newcode83
options.html:83: <img src="skin/tooltips/block.png">
I assume we are not adding this images to sprite, because we expect them to be
replaceable in future ?

https://codereview.adblockplus.org/29322581/diff/29322582/options.html#newcode83
options.html:83: <img src="skin/tooltips/block.png">
Nit: please add closing slash for consistency.

https://codereview.adblockplus.org/29322581/diff/29322582/options.html#newcode83
options.html:83: <img src="skin/tooltips/block.png">
Please add alt attribute.

https://codereview.adblockplus.org/29322581/diff/29322582/options.html#newcode86
options.html:86: <div
class="i18n_options_language_title_tooltip_bottom_1"></div>
I think they behave as paragraph here, so I think make sense to use P tag here.

https://codereview.adblockplus.org/29322581/diff/29322582/options.html#newcod...
options.html:108: <span class="tooltip">
Same as above, this should be div. 
I'll stop commenting for all similar cases that covered above, because looks
like most of them applies to each tooltip block.

https://codereview.adblockplus.org/29322581/diff/29322582/options.html#newcod...
options.html:151: <div class="i18n_options_acceptableAds_title_tooltip_1"></div>
Here again tooltips behave as paragraph or heading :/
I don't have strong opinion here because the tooltips are bit different.
So in case they will stay so you can use here whatever element you want, but
maybe we can describe them somehow with role attribute maybe, anyway we can
rethink regarding that for accessibility issue.

https://codereview.adblockplus.org/29322581/diff/29322582/skin/options.css
File skin/options.css (right):

https://codereview.adblockplus.org/29322581/diff/29322582/skin/options.css#ne...
skin/options.css:108: -moz-margin-end: auto;
Why do we need "margin-end: auto;" here ?

https://codereview.adblockplus.org/29322581/diff/29322582/skin/options.css#ne...
skin/options.css:149: .flex-container > div
I do think this rule adding confusion, because this is something that is
specific for general tab, while the class has general name and general role.

I'd rename flex-container and had something like this:
#content-general .flex-row > div

https://codereview.adblockplus.org/29322581/diff/29322582/skin/options.css#ne...
skin/options.css:763: .tooltip, #block-element-explanation a
Why don't just add class ".tooltip" to "#block-element-explanation a" element ?
Anyway it's not specified that we need tooltip there in current ticket, so it
can be handled, in following reviews.

https://codereview.adblockplus.org/29322581/diff/29322582/skin/options.css#ne...
skin/options.css:800: max-width: 400px;
Why we use both max and min width ? 
Please use "width: 400px;"

https://codereview.adblockplus.org/29322581/diff/29322582/skin/options.css#ne...
skin/options.css:805: transition: opacity 200ms ease-in-out 400ms;
In case we will need to support safari 6.0 we will need to add "-webkit", so
what is the plan are we going to drop it ?
If no please also specify webkit prefix.

https://codereview.adblockplus.org/29322581/diff/29322582/skin/options.css#ne...
skin/options.css:818: div[role="tooltip"]:hover
On small screens some tooltips overflow this element and as result tooltips are
not shown.

[Thomas Greiner, 2015-07-28 14:13:08]

https://codereview.adblockplus.org/29322581/diff/29322582/options.html
File options.html (right):

https://codereview.adblockplus.org/29322581/diff/29322582/options.html#newcode80
options.html:80: <span class="tooltip">
On 2015/07/22 18:25:38, saroyanm wrote:
> I think we shouldn't put div inside span, until we really need it, actually
> can't find even case why we would need it.

Done.

https://codereview.adblockplus.org/29322581/diff/29322582/options.html#newcode83
options.html:83: <img src="skin/tooltips/block.png">
On 2015/07/22 18:25:38, saroyanm wrote:
> Please add alt attribute.

Done. Note that I intentionally left it empty since it's only a decorative image
and is therefore not necessary for navigating the page. See also
http://www.w3.org/WAI/tutorials/images/decorative/

https://codereview.adblockplus.org/29322581/diff/29322582/options.html#newcode83
options.html:83: <img src="skin/tooltips/block.png">
On 2015/07/22 18:25:38, saroyanm wrote:
> I assume we are not adding this images to sprite, because we expect them to be
> replaceable in future ?

No, because we don't need to. A sprite is a workaround for situations when you
(a) have limited bandwidth or high latency or (b) when you want to make sure
that images are loaded before making them visible to avoid flashing.

Therefore sprites usually make sense for small icons that are used repeatedly
across the page. In our case, however, (a) is not a limitation and for (b) it's
not necessary since we have the transition delay in there anyway.

https://codereview.adblockplus.org/29322581/diff/29322582/options.html#newcode83
options.html:83: <img src="skin/tooltips/block.png">
On 2015/07/22 18:25:38, saroyanm wrote:
> Nit: please add closing slash for consistency.

Done.

https://codereview.adblockplus.org/29322581/diff/29322582/options.html#newcode86
options.html:86: <div
class="i18n_options_language_title_tooltip_bottom_1"></div>
On 2015/07/22 18:25:38, saroyanm wrote:
> I think they behave as paragraph here, so I think make sense to use P tag
here.

Done.

https://codereview.adblockplus.org/29322581/diff/29322582/options.html#newcod...
options.html:108: <span class="tooltip">
On 2015/07/22 18:25:38, saroyanm wrote:
> Same as above, this should be div. 
> I'll stop commenting for all similar cases that covered above, because looks
> like most of them applies to each tooltip block.

Done.

https://codereview.adblockplus.org/29322581/diff/29322582/options.html#newcod...
options.html:151: <div class="i18n_options_acceptableAds_title_tooltip_1"></div>
On 2015/07/22 18:25:38, saroyanm wrote:
> Here again tooltips behave as paragraph or heading :/
> I don't have strong opinion here because the tooltips are bit different.
> So in case they will stay so you can use here whatever element you want, but
> maybe we can describe them somehow with role attribute maybe, anyway we can
> rethink regarding that for accessibility issue.

Done.

https://codereview.adblockplus.org/29322581/diff/29322582/skin/options.css
File skin/options.css (right):

https://codereview.adblockplus.org/29322581/diff/29322582/skin/options.css#ne...
skin/options.css:108: -moz-margin-end: auto;
On 2015/07/22 18:25:38, saroyanm wrote:
> Why do we need "margin-end: auto;" here ?

Done. Seems to be redundant when using `flex: 1`.

https://codereview.adblockplus.org/29322581/diff/29322582/skin/options.css#ne...
skin/options.css:149: .flex-container > div
On 2015/07/22 18:25:38, saroyanm wrote:
> I do think this rule adding confusion, because this is something that is
> specific for general tab, while the class has general name and general role.
> 
> I'd rename flex-container and had something like this:
> #content-general .flex-row > div

I'd rather avoid having implementation details such as "flex" in the class name
so I called it "row-bisect" since that's what it does.

I don't see a reason, however, to make it specific to the General tab.

https://codereview.adblockplus.org/29322581/diff/29322582/skin/options.css#ne...
skin/options.css:763: .tooltip, #block-element-explanation a
On 2015/07/22 18:25:39, saroyanm wrote:
> Why don't just add class ".tooltip" to "#block-element-explanation a" element
?
> Anyway it's not specified that we need tooltip there in current ticket, so it
> can be handled, in following reviews.

Yes, especially since this part of the page has not been implemented yet. This
should be tackled as part of #2802 because those `<a>` tags are part of the
translatable text. Therefore this tooltip needs to be added dynamically.

I added a note to #2802 to make sure that this case is taken into consideration.

https://codereview.adblockplus.org/29322581/diff/29322582/skin/options.css#ne...
skin/options.css:800: max-width: 400px;
On 2015/07/22 18:25:38, saroyanm wrote:
> Why we use both max and min width ? 
> Please use "width: 400px;"

Done.

https://codereview.adblockplus.org/29322581/diff/29322582/skin/options.css#ne...
skin/options.css:805: transition: opacity 200ms ease-in-out 400ms;
On 2015/07/22 18:25:39, saroyanm wrote:
> In case we will need to support safari 6.0 we will need to add "-webkit", so
> what is the plan are we going to drop it ?
> If no please also specify webkit prefix.

According to MDN, `transition-delay` is still prefixed in newer versions of
Safari so we won't be able to avoid prefixes anyway.

https://codereview.adblockplus.org/29322581/diff/29322582/skin/options.css#ne...
skin/options.css:818: div[role="tooltip"]:hover
On 2015/07/22 18:25:39, saroyanm wrote:
> On small screens some tooltips overflow this element and as result tooltips
are
> not shown.

Done. Nice catch!

[saroyanm, 2015-07-28 17:21:43]

BTW I do think make sense to have special folder for images, currently we have
images alongside with "css", I think make sense to also move all images into
/skin/images/ with separate issue, maybe make sense to add it to the "TODO"
document ?

https://codereview.adblockplus.org/29322581/diff/29322582/options.html
File options.html (right):

https://codereview.adblockplus.org/29322581/diff/29322582/options.html#newcode83
options.html:83: <img src="skin/tooltips/block.png">
On 2015/07/28 14:13:06, Thomas Greiner wrote:
> On 2015/07/22 18:25:38, saroyanm wrote:
> > I assume we are not adding this images to sprite, because we expect them to
be
> > replaceable in future ?
> 
> No, because we don't need to. A sprite is a workaround for situations when you
> (a) have limited bandwidth or high latency or (b) when you want to make sure
> that images are loaded before making them visible to avoid flashing.
> 
> Therefore sprites usually make sense for small icons that are used repeatedly
> across the page. In our case, however, (a) is not a limitation and for (b)
it's
> not necessary since we have the transition delay in there anyway.

Fare enough

https://codereview.adblockplus.org/29322581/diff/29322582/options.html#newcode83
options.html:83: <img src="skin/tooltips/block.png">
On 2015/07/28 14:13:06, Thomas Greiner wrote:
> On 2015/07/22 18:25:38, saroyanm wrote:
> > Please add alt attribute.
> 
> Done. Note that I intentionally left it empty since it's only a decorative
image
> and is therefore not necessary for navigating the page. See also
> http://www.w3.org/WAI/tutorials/images/decorative/

Sure, but we still need required attribute even if they can be empty, thanks for
the link, good to know.

https://codereview.adblockplus.org/29322581/diff/29322582/skin/options.css
File skin/options.css (right):

https://codereview.adblockplus.org/29322581/diff/29322582/skin/options.css#ne...
skin/options.css:149: .flex-container > div
On 2015/07/28 14:13:07, Thomas Greiner wrote:
> On 2015/07/22 18:25:38, saroyanm wrote:
> > I do think this rule adding confusion, because this is something that is
> > specific for general tab, while the class has general name and general role.
> > 
> > I'd rename flex-container and had something like this:
> > #content-general .flex-row > div
> 
> I'd rather avoid having implementation details such as "flex" in the class
name
> so I called it "row-bisect" since that's what it does.
> 
> I don't see a reason, however, to make it specific to the General tab.

Well it does bisect the row, but why each child div has width of 400px ?
It feels like it's specific for general tab.
(I was thinking about case when we will need to have a new tab with 4 items in
the row so we can specify style for "row-bisect" to 200px for that new tab, so
it will be a generic style.)

I think it's personal preference while we don't using this style on other tabs,
but if you have another argument about this specific style, would be nice to
take into account, otherwise it's okey as it is while it's consistent with the
implementation.

https://codereview.adblockplus.org/29322581/diff/29322854/skin/options.css
File skin/options.css (right):

https://codereview.adblockplus.org/29322581/diff/29322854/skin/options.css#ne...
skin/options.css:819: div[role="tooltip"]:hover
why we need "div[role="tooltip"]:hover" selector here ? 

I think the first selector should specify the transition-delay and do the magic.

[Thomas Greiner, 2015-07-28 18:19:23]

https://codereview.adblockplus.org/29322581/diff/29322582/options.html
File options.html (right):

https://codereview.adblockplus.org/29322581/diff/29322582/options.html#newcode83
options.html:83: <img src="skin/tooltips/block.png">
On 2015/07/28 17:21:42, saroyanm wrote:
> On 2015/07/28 14:13:06, Thomas Greiner wrote:
> > On 2015/07/22 18:25:38, saroyanm wrote:
> > > Please add alt attribute.
> > 
> > Done. Note that I intentionally left it empty since it's only a decorative
> image
> > and is therefore not necessary for navigating the page. See also
> > http://www.w3.org/WAI/tutorials/images/decorative/
> 
> Sure, but we still need required attribute even if they can be empty, thanks
for
> the link, good to know.

Yeah, that's what I did.

https://codereview.adblockplus.org/29322581/diff/29322582/skin/options.css
File skin/options.css (right):

https://codereview.adblockplus.org/29322581/diff/29322582/skin/options.css#ne...
skin/options.css:149: .flex-container > div
On 2015/07/28 17:21:42, saroyanm wrote:
> On 2015/07/28 14:13:07, Thomas Greiner wrote:
> > On 2015/07/22 18:25:38, saroyanm wrote:
> > > I do think this rule adding confusion, because this is something that is
> > > specific for general tab, while the class has general name and general
role.
> > > 
> > > I'd rename flex-container and had something like this:
> > > #content-general .flex-row > div
> > 
> > I'd rather avoid having implementation details such as "flex" in the class
> name
> > so I called it "row-bisect" since that's what it does.
> > 
> > I don't see a reason, however, to make it specific to the General tab.
> 
> Well it does bisect the row, but why each child div has width of 400px ?
> It feels like it's specific for general tab.
> (I was thinking about case when we will need to have a new tab with 4 items in
> the row so we can specify style for "row-bisect" to 200px for that new tab, so
> it will be a generic style.)
> 
> I think it's personal preference while we don't using this style on other
tabs,
> but if you have another argument about this specific style, would be nice to
> take into account, otherwise it's okey as it is while it's consistent with the
> implementation.

I see your point. Hardcoding it to 400px is a bad practice anyway so I made it
more flexible now. For that I also had to correct the width of the tab content
container element which was exceeding the 960px that are specified in the style
guide.

Note that I chose the class name "hbox" based on XUL's `<hbox>` element which
behaves similarly to this (see
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/hbox).

https://codereview.adblockplus.org/29322581/diff/29322854/skin/options.css
File skin/options.css (right):

https://codereview.adblockplus.org/29322581/diff/29322854/skin/options.css#ne...
skin/options.css:819: div[role="tooltip"]:hover
On 2015/07/28 17:21:42, saroyanm wrote:
> why we need "div[role="tooltip"]:hover" selector here ? 

Because when you hover over the tooltip it should disappear. Usually, it would
stay visible because by hovering over the DIV, you're also hovering over its
parent element which in this case is `.tooltip`.

> I think the first selector should specify the transition-delay and do the
magic.

It already does set a transition delay of 400ms which causes the tooltip to
become visible delayed by 400ms. This one, however, changes the transition delay
to 0ms causing it to disappear instantly instead of after 400ms.

[saroyanm, 2015-07-29 09:39:00]

https://codereview.adblockplus.org/29322581/diff/29322854/skin/options.css
File skin/options.css (right):

https://codereview.adblockplus.org/29322581/diff/29322854/skin/options.css#ne...
skin/options.css:819: div[role="tooltip"]:hover
On 2015/07/28 18:19:22, Thomas Greiner wrote:
> On 2015/07/28 17:21:42, saroyanm wrote:
> > why we need "div[role="tooltip"]:hover" selector here ? 
> 
> Because when you hover over the tooltip it should disappear. Usually, it would
> stay visible because by hovering over the DIV, you're also hovering over its
> parent element which in this case is `.tooltip`.
> 
> > I think the first selector should specify the transition-delay and do the
> magic.
> 
> It already does set a transition delay of 400ms which causes the tooltip to
> become visible delayed by 400ms. This one, however, changes the transition
delay
> to 0ms causing it to disappear instantly instead of after 400ms.

My mistake.

https://codereview.adblockplus.org/29322581/diff/29322863/skin/options.css
File skin/options.css (right):

https://codereview.adblockplus.org/29322581/diff/29322863/skin/options.css#ne...
skin/options.css:149: flex: 1;
In this case "flex-wrap: wrap" will have no effect.
I think we need to specify min-width.

[Thomas Greiner, 2015-07-29 09:59:53]

https://codereview.adblockplus.org/29322581/diff/29322863/skin/options.css
File skin/options.css (right):

https://codereview.adblockplus.org/29322581/diff/29322863/skin/options.css#ne...
skin/options.css:149: flex: 1;
On 2015/07/29 09:38:59, saroyanm wrote:
> In this case "flex-wrap: wrap" will have no effect.
> I think we need to specify min-width.

There's no wrapping necessary in the current version. Therefore I'd suggest
tackling that in #2746. Removed `flex-wrap` for now.

Also added `min-width` to the tab content container for now so that the design
doesn't look too screwed up on smaller screens until we get around tackling
#2746.

[saroyanm, 2015-07-29 10:12:12]

On 2015/07/29 09:59:53, Thomas Greiner wrote:
> https://codereview.adblockplus.org/29322581/diff/29322863/skin/options.css
> File skin/options.css (right):
> 
>
https://codereview.adblockplus.org/29322581/diff/29322863/skin/options.css#ne...
> skin/options.css:149: flex: 1;
> On 2015/07/29 09:38:59, saroyanm wrote:
> > In this case "flex-wrap: wrap" will have no effect.
> > I think we need to specify min-width.
> 
> There's no wrapping necessary in the current version. Therefore I'd suggest
> tackling that in #2746. Removed `flex-wrap` for now.
> 
> Also added `min-width` to the tab content container for now so that the design
> doesn't look too screwed up on smaller screens until we get around tackling
> #2746.

LGTM

[saroyanm, 2015-07-29 10:12:21]

https://codereview.adblockplus.org/29322581/diff/29322872/skin/options.css
File skin/options.css (right):

https://codereview.adblockplus.org/29322581/diff/29322872/skin/options.css#ne...
skin/options.css:268: width: 960px;
nit: It's doesn't make any sense to have specified width here, but as far as I
understand this styles will be changed anyway so feel free keep it or remove so
we will come back to it later on.