Log element hiding exceptions in the developer tools panel

[kzar]

Notes

Now that I have an actual understanding of this change I realize this is in fact a platform ticket, which will induce a change to the spec but is not actually UI dependent in any way. Rather than muddy this ticket further I will leave it is a reference to the gitlab issues for platform​(#4), and eyeo/specs/spec​(#206)

sorry for the confusion and thanks for all the input!

[sporz]

+1, this is a blocker for several analyses we'd like to do rather sooner than later in BizDev and AA Monitoring.

[greiner]

Unsetting the ready flag until the spec is done and ready for implementation, because we decided to turn this into a UI ticket.

[sebastian]

I don't see any required changes in adblockplusui here (other than maybe mentioning element hiding exceptions in the spec, and including an example when using the mock implementation). But as far are as I understand, as soon as we pass records for element hiding exceptions from adblockpluschrome to the devtools panel's UI, they should just show up like every other filter does, or what am I missing?

[jsonesen]

Replying to sebastian:

I don't see any required changes in adblockplusui here (other than maybe mentioning element hiding exceptions in the spec, and including an example when using the mock implementation).

I think you are right; once the platform returns the exception filters then it should be handled by the devtools panel like any other record. I did not consider that when meeting with Thomas about this (it came up as a side note regarding #7401) but also, I too may be missing something.

[greiner]

As discussed with Jon, as long as we don't pass any new filters to the UI, we can safely go ahead with any Platform changes.

It's currently undefined behavior how the UI handles any such filters though so if we do end up passing them along, we should check how the UI reacts and whether we are satisfied with how it behaves. Otherwise we should wait until we've made the necessary UI changes before applying the Platform changes.

[sebastian]

Sure, we have to test it, and if for any reasons the combination of element hiding + exception rule breaks the UI of the devtools panel, this needs to be addressed. But if it works just OK (and I don't see any reason to assume otherwise), I would rather not block this change by another UI update.

With your line of argument, we would not be able to add any new type of filter without running that first by the UI team, in order to make sure they show up as specified in the devtools panel. That is not going to happen. Maybe you should consider to make the spec of the devtools panel UI more abstract, so that rather than specifying how exactly a specific filter is rendered, describe individual features like element hiding and whitelisting and how they are meant to change the representation of a record in the devtools panel. Then the case of element hiding exceptions wouldn't be unspecified.

[jsonesen]

I'd also like to point out, that while this ticket can be landed without effecting ui code the elemhide exceptions render red but should be green and that is the only difference in the spec/ui code. I'd say we can land this without first updating g the spec and everything else (mostly since this ticket has been sitting over a year)

[greiner]

Replying to sebastian:

Sure, we have to test it, and if for any reasons the combination of element hiding + exception rule breaks the UI of the devtools panel, this needs to be addressed. But if it works just OK (and I don't see any reason to assume otherwise), I would rather not block this change by another UI update.

Of course, there should be no regressions which means nothing should break or be shown in an inconsistent manner.

With your line of argument, we would not be able to add any new type of filter without running that first by the UI team, in order to make sure they show up as specified in the devtools panel. That is not going to happen.

That's one option. The other one being that no changes will propagate to the UI before the necessary changes have been made. In any case we should avoid UI regressions as much as we should avoid regressions in other parts of the extension.

Fortunately, that shouldn't block anything though.

Maybe you should consider to make the spec of the devtools panel UI more abstract, so that rather than specifying how exactly a specific filter is rendered, describe individual features like element hiding and whitelisting and how they are meant to change the representation of a record in the devtools panel. Then the case of element hiding exceptions wouldn't be unspecified.

The current spec was created based on what has already been implemented. No discussions have happened yet around how we'd like the UI to behave or look like.

Replying to jsonesen:

I'd also like to point out, that while this ticket can be landed without effecting ui code the elemhide exceptions render red but should be green and that is the only difference in the spec/ui code.

Thanks for checking. Does that mean that such filters are also shown when selecting only "blocked" items in the list to be shown?

[jsonesen]

Replying to greiner:

Thanks for checking. Does that mean that such filters are also shown when selecting only "blocked" items in the list to be shown?

Indeed this is how it works right now, however I pretty much figured out how to get the exceptions to the devtools panel and went to bed. I think it makes sense to actually move this ticket back to platform and open a different UI ticket which includes the spec update etc.

What do you think?

If so I can also take the opportunity to migrate this as a platform ticket to gitlab, and open a new one on the ui and spec gitlab.

[sebastian]

Replying to jsonesen:

I'd also like to point out, that while this ticket can be landed without effecting ui code the elemhide exceptions render red but should be green and that is the only difference in the spec/ui code.

Since I didn't see your code yet, I'm just guessing, but to me that rather seems like a bug in adblockpluschrome. The UI treats the record as an exception rule (e.g. renders it green) if the whitelisted property in the request info object passed from adblockpluschrome is true.

Replying to jsonesen:

I think it makes sense to actually move this ticket back to platform and open a different UI ticket which includes the spec update etc.

What do you think?

If so I can also take the opportunity to migrate this as a platform ticket to gitlab, and open a new one on the ui and spec gitlab.

Yeah, as discussed on IRC, let's file a new issue for adblockpluschrome on GitLab, and then leave it up to Thomas what to do with this issue.

[sebastian]

This issue is being continued as ​chrome#4 on GitLab.

So I'm closing this one as duplicate.