Opened 22 months ago

Closed 8 months ago

#6427 closed change (duplicate)

Log element hiding exceptions in the developer tools panel

Reported by: kzar Assignee: jsonesen
Priority: P2 Milestone:
Module: Platform Keywords:
Cc: Ross, sebastian, mapx, arthur, greiner, sporz, mjethani Blocked By: #6428
Blocking: Platform: Chrome
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by jsonesen)

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!

Change History (21)

comment:1 Changed 22 months ago by kzar

  • Blocked By 6428 added

comment:2 Changed 22 months ago by greiner

  • Cc greiner added

comment:3 Changed 19 months ago by kzar

  • Ready set

comment:4 Changed 19 months ago by jsonesen

  • Owner set to jsonesen

comment:5 Changed 10 months ago by sporz

  • Cc sporz added

comment:6 Changed 10 months ago by sporz

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

comment:7 Changed 9 months ago by philll

Last edited 9 months ago by philll (previous) (diff)

comment:8 Changed 8 months ago by jsonesen

  • Component changed from Platform to User-Interface
  • Description modified (diff)

comment:9 Changed 8 months ago by jsonesen

  • Description modified (diff)

comment:10 Changed 8 months ago by greiner

  • Ready unset

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

comment:11 follow-up: Changed 8 months ago by sebastian

  • Cc mjethani added

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?

comment:12 in reply to: ↑ 11 Changed 8 months ago by 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.

comment:13 Changed 8 months ago by mjethani

  • Description modified (diff)

comment:14 Changed 8 months ago by 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.

comment:15 Changed 8 months ago by 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.

Last edited 8 months ago by sebastian (previous) (diff)

comment:16 follow-up: Changed 8 months ago by 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)

comment:17 in reply to: ↑ 16 ; follow-up: Changed 8 months ago by 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?

comment:18 in reply to: ↑ 17 ; follow-up: Changed 8 months ago by 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.

comment:19 in reply to: ↑ 18 Changed 8 months ago by 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.

comment:20 Changed 8 months ago by jsonesen

  • Component changed from User-Interface to Platform
  • Description modified (diff)

comment:21 Changed 8 months ago by sebastian

  • Resolution set to duplicate
  • Status changed from new to closed

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

So I'm closing this one as duplicate.

Note: See TracTickets for help on using tickets.