Refactor how the CBH panel item maps application state to DOM attributes
Categories
(Core :: Privacy: Anti-Tracking, task, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox113 | --- | fixed |
People
(Reporter: jhirsch, Assigned: jhirsch)
References
(Blocks 1 open bug)
Details
(Whiteboard: [fidefe-cookie-banner-v2])
Attachments
(1 file)
In the patch[1] for bug 1800670, the shield panel menu item state is awkwardly mapped to a number of DOM attributes: uiDisabled
, hasException
, enabled
and disabled
.
It would be simpler to just have one data-attribute reflecting the CBH state as a string, and adjust the CSS selectors and visibility based on matching the selector.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Assignee | ||
Comment 1•2 years ago
|
||
Rebased, but commit message not cleaned up yet.
Bug 1812796 - UI state refactoring part 1: replace uiDisabled DOM attribute with hidden DOM attribute.
Refactor display logic for the cookie banner menu item
Waypoint commit: refactored CSS, but haven't yet tested it.
The goal here is to make the code easier to reason about by
conditionally showing the appropriate UI, not hiding the other
two states. And consolidating all the state into one string
valued attribute.
TODO: if the subview is not a child of the CBH section, then
we will need to set state in two places. I dimly remember
finding this out the hard way last time.
I think cleanup may nearly be complete
Time to test it out in the build
Yup, the subview is not a child of the menu item. Had to set state in two places.
also spotted an updateSubview method I need to clean up still.
Remove the no-longer-used hasException attribute from the subView
I'd like to just merge updateSubview into updateSection above, but since
every other subview has this method in its API, maybe the right thing
actually is to have a helper method that calculates the correct value
of the state attribute and returns it as a string. That would enable us
to set the state in the expected place, but also avoid duplicating the
logic used to determine the state and map it to the correct UI attribute
value.
Refactoring complete, now with a unified state calculator method for subview and menu item.
As mentioned in previous commit, we unfortunately have to set state in
two places in the DOM, and by convention, the subview state is
calculated in a different method and at a different time than the menu
item. To avoid duplicating the UI state calculation logic, we extract
the logic into one helper method which returns the state value as a
string, and then both subview and menu item set that value on a
data-attribute used by the CSS to conditionally show different parts of
the UI.
I dunno how I'm going to summarize this concisely, but that's a problem
for later.
delete old CSS
Update tests
Remove some leftovers. TODO should we really use a dataset here? or just a plain attribute?
A few more simplifications
-
Simplify the updateSubview method by just storing a pointer to the
enable and disable descriptions, and updating them using a simple
getter. -
Simplify shouldShowSection logic, including renaming this._uiDisabled
to this._uiEnabled and consolidating the early return logic. -
Remove some stray leftover dead code
Updated•2 years ago
|
Comment 3•2 years ago
|
||
bugherder |
Description
•