Issue 2802/2358 - Dynamically generate tooltips in options page

Issue 2802/2358 - Dynamically generate tooltips in options page

[Thomas Greiner, 2016-01-19 19:28:44]

https://codereview.adblockplus.org/29334038/diff/29334039/skin/options.css
File skin/options.css (left):

https://codereview.adblockplus.org/29334038/diff/29334039/skin/options.css#ol...
skin/options.css:401: }
Please ignore this change. It's already covered by
https://codereview.adblockplus.org/29333262/

[saroyanm, 2016-03-21 10:04:13]

Thomas can you please rebase this review when you will have time and it will be
ready for review and ping me ?

[Thomas Greiner, 2016-03-21 14:21:27]

On 2016/03/21 10:04:13, saroyanm wrote:
> Thomas can you please rebase this review when you will have time and it will
be
> ready for review and ping me ?

Done.

[saroyanm, 2016-03-24 18:27:51]

https://codereview.adblockplus.org/29334038/diff/29338813/options.html
File options.html (right):

https://codereview.adblockplus.org/29334038/diff/29338813/options.html#newcod...
options.html:115: <div class="fill">
Why do we need this fill wrapper ?

https://codereview.adblockplus.org/29334038/diff/29338813/options.js
File options.js (right):

https://codereview.adblockplus.org/29334038/diff/29338813/options.js#newcode382
options.js:382: updateTooltips();
Why do you update tooltip on every update ? I think this should be called during
the load and after adding new subscription.

https://codereview.adblockplus.org/29334038/diff/29338813/options.js#newcode486
options.js:486: var recommendation = {type: element.getAttribute("type")};
Do we need reccomendationMap ? I think it's possible to only have type in
"knownSubscriptions" anyway we are not overriding the properties until the
original subscriptions doesn't have some, so I assume we can get rid of
recommendationsMap and keep checking for type as you did above, beforehand we
were overriding all properties, so probably that was the reason why we had two
different mapping objects.
Otherwise I think it's better to have types only in recommendationMaps.

https://codereview.adblockplus.org/29334038/diff/29338813/options.js#newcode1103
options.js:1103: var anchor = anchors[i];
Why does it called anchor ?

https://codereview.adblockplus.org/29334038/diff/29338813/options.js#newcode1105
options.js:1105: id = id.replace("%value%",
anchor.getAttribute("data-tooltip-value"));
Why not to do this when the item is added ? 
Currently you are setting there the data-tooltip-value and barely using it, I'm
not sure if we even need this attribute ?

https://codereview.adblockplus.org/29334038/diff/29338813/options.js#newcode1107
options.js:1107: var wrapper = document.createElement("div");
Why do we need create a wrapper for elements that are only using title attribute
to show the tooltip, I think we should only do that for the elements that really
need wrapper.

https://codereview.adblockplus.org/29334038/diff/29338813/options.js#newcode1131
options.js:1131: var imageSource = anchor.getAttribute("data-tooltip-image");
According to the ticket image attribute should be retrieved using [data-tooltip]
attribute.

[Thomas Greiner, 2016-05-31 17:00:02]

https://codereview.adblockplus.org/29334038/diff/29338813/options.html
File options.html (right):

https://codereview.adblockplus.org/29334038/diff/29338813/options.html#newcod...
options.html:115: <div class="fill">
On 2016/03/24 18:27:50, saroyanm wrote:
> Why do we need this fill wrapper ?

To fill the space between the checkbox and the "popular" label. Since the latter
will be removed in #3741, it won't be necessary anymore though.

https://codereview.adblockplus.org/29334038/diff/29338813/options.js
File options.js (right):

https://codereview.adblockplus.org/29334038/diff/29338813/options.js#newcode382
options.js:382: updateTooltips();
On 2016/03/24 18:27:51, saroyanm wrote:
> Why do you update tooltip on every update ? I think this should be called
during
> the load and after adding new subscription.

Since we no longer use `observeSubscription()` I made that change now together
with the rebase.

https://codereview.adblockplus.org/29334038/diff/29338813/options.js#newcode486
options.js:486: var recommendation = {type: element.getAttribute("type")};
On 2016/03/24 18:27:51, saroyanm wrote:
> Do we need reccomendationMap ? I think it's possible to only have type in
> "knownSubscriptions" anyway we are not overriding the properties until the
> original subscriptions doesn't have some, so I assume we can get rid of
> recommendationsMap and keep checking for type as you did above, beforehand we
> were overriding all properties, so probably that was the reason why we had two
> different mapping objects.
> Otherwise I think it's better to have types only in recommendationMaps.

Indeed, `recommendationsMap` was no longer necessary so I removed it now.

https://codereview.adblockplus.org/29334038/diff/29338813/options.js#newcode1103
options.js:1103: var anchor = anchors[i];
On 2016/03/24 18:27:50, saroyanm wrote:
> Why does it called anchor ? 

Because it's the element the tooltip is anchored/attached to.

You can also find this naming in the XUL's `openPopup()` function which opens a
popup that's anchored to an existing UI element (see
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Method/openPopup).
It's also why the `<a>` tag is called "anchor tag" (see
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a).

https://codereview.adblockplus.org/29334038/diff/29338813/options.js#newcode1105
options.js:1105: id = id.replace("%value%",
anchor.getAttribute("data-tooltip-value"));
On 2016/03/24 18:27:50, saroyanm wrote:
> Why not to do this when the item is added ? 
> Currently you are setting there the data-tooltip-value and barely using it,
I'm
> not sure if we even need this attribute ?

Done.

https://codereview.adblockplus.org/29334038/diff/29338813/options.js#newcode1107
options.js:1107: var wrapper = document.createElement("div");
On 2016/03/24 18:27:50, saroyanm wrote:
> Why do we need create a wrapper for elements that are only using title
attribute
> to show the tooltip, I think we should only do that for the elements that
really
> need wrapper.

Because the alternative would be to put the tooltip inside of the anchor
element, in which case the tooltip would point to itself if we use
"aria-describedby" or "for" later on and that may cause issues (see #2411).

https://codereview.adblockplus.org/29334038/diff/29338813/options.js#newcode1131
options.js:1131: var imageSource = anchor.getAttribute("data-tooltip-image");
On 2016/03/24 18:27:51, saroyanm wrote:
> According to the ticket image attribute should be retrieved using
[data-tooltip]
> attribute.

It's just tricky to determine whether the image exists so I thought it'd be
better to specify it explicitly since not every tooltip has an image.

[saroyanm, 2016-06-08 15:48:32]

LGTM

https://codereview.adblockplus.org/29334038/diff/29338813/options.html
File options.html (right):

https://codereview.adblockplus.org/29334038/diff/29338813/options.html#newcod...
options.html:115: <div class="fill">
On 2016/05/31 17:00:01, Thomas Greiner wrote:
> On 2016/03/24 18:27:50, saroyanm wrote:
> > Why do we need this fill wrapper ?
> 
> To fill the space between the checkbox and the "popular" label. Since the
latter
> will be removed in #3741, it won't be necessary anymore though.

Got it, yes make sense to take care of that during rebasing if this change will
land earlier.