Closed Bug 1812796 Opened 2 years ago Closed 2 years ago

Refactor how the CBH panel item maps application state to DOM attributes

Categories

(Core :: Privacy: Anti-Tracking, task, P3)

task
Points:
2

Tracking

()

RESOLVED FIXED
113 Branch
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.

[1] https://phabricator.services.mozilla.com/D164241

Severity: -- → S4
Type: enhancement → task
Points: --- → 2
Priority: -- → P3
Whiteboard: [fidefe-cookie-banner-v2]
Assignee: nobody → jhirsch

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

Attachment #9322005 - Attachment description: WIP: Bug 1812796 - Refactor and simplify cookie banner handling UI state → Bug 1812796 - Refactor and simplify cookie banner handling UI state r?pbz
Pushed by jhirsch@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/779d9f9aff63 Refactor and simplify cookie banner handling UI state r=pbz,desktop-theme-reviewers,Itiel
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 113 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: