Opened 8 months ago

Closed 8 months ago

Last modified 8 months ago

#7430 closed defect (fixed)

UI not getting notified of change when toggling preference

Reported by: greiner Assignee: kzar
Priority: P2 Milestone: Adblock-Plus-for-Chrome-Opera-Firefox-next
Module: Platform Keywords:
Cc: sebastian, kzar, Ross Blocked By:
Blocking: Platform: Firefox
Ready: yes Confidential: no
Tester: Unknown Verified working: yes
Review URL(s):

https://gitlab.com/eyeo/adblockplus/adblockpluschrome/merge_requests/62

Description (last modified by kzar)

Environment

Ubuntu 16.04 / Windows
Firefox 66
Adblock Plus 3.5

How to reproduce

  1. Open Advanced tab in options page
  2. Click on "Show 'Adblock Plus' panel in developer tools" checkbox
  3. Click on checkbox again

Observed behaviour

After step 2) Checkbox is unchecked and preference is set to false.
After step 3) Checkbox remains unchecked and preference is removed.

Expected behaviour

After step 2) Checkbox is unchecked and preference is set to false.
After step 3) Checkbox is checked and preference is removed.

Further information

Upon further investigation it seems that browser.storage.onChanged no longer notifies us when removing a preference whose value is false.

See this Firefox bug report.

See also ui#384.

Hints for testers

  • The underlying Firefox bug has now been fixed and that fix included with Firefox 67. Test that our workaround here works for Firefox 66. Please also test that the options page and other UI relating to user preferences works OK in different versions of Firefox, also on Chrome.

Change History (14)

comment:1 Changed 8 months ago by kzar

I've just read through this and the linked GitLab issue, just to confirm you can't reproduce the problem in earlier versions of Firefox, nor for other platforms? If not, I wonder if this is a regression in Firefox itself.

comment:2 Changed 8 months ago by greiner

According to Ursa "older versions work as expected" so it could very well be a regression by Firefox. I don't know about other browsers.

comment:3 Changed 8 months ago by kzar

Just thinking out loud, but perhaps a workaround could be to store a different "falsey" value instead of false? For example, does the event fire for 0?

comment:4 Changed 8 months ago by greiner

Here's the data I gathered so far:

Value Value (initial) onChanged Value (set) onChanged Value (removed)
"foo" undefined undefined -> "foo" "foo" "foo" -> undefined undefined
"" undefined undefined -> "" "" (missing) undefined
true undefined undefined -> true true true -> undefined undefined
false undefined undefined -> false false (missing) undefined
1 undefined undefined -> 1 1 1 -> undefined undefined
0 undefined undefined -> 0 0 (missing) undefined
null undefined undefined -> null null (missing) undefined
undefined undefined undefined -> undefined undefined (missing) undefined

Therefore it seems that this issue applies to all falsey values.

PS: I gathered the data using a simple extension that runs the following for any of the given values and logs out the various data while doing so:

  • browser.storage.local.set({foo: <value>})
  • browser.storage.local.remove("foo")

comment:5 Changed 8 months ago by sebastian

  • Priority changed from Unknown to P2
  • Ready set

Can we please file a Firefox bug?

We should also implement a workaround.

comment:6 Changed 8 months ago by ukacar

Just to confirm that this issue only happens in Firefox 66 (and 67, which is the current Beta version). Chrome, Opera and older versions of Firefox work as expected. :)

comment:7 Changed 8 months ago by kzar

  • Description modified (diff)

Can we please file a Firefox bug?

Done that now, see https://bugzilla.mozilla.org/show_bug.cgi?id=1541449

comment:9 Changed 8 months ago by kzar

  • Owner set to kzar

comment:10 Changed 8 months ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:12 Changed 8 months ago by kzar

  • Cc Ross added
  • Description modified (diff)
  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next

Note: This milestone is wrong, since this workaround is going to be included in the upcoming hotfix release. Once the correct milestone exists, we can update it here.

comment:13 Changed 8 months ago by kzar

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:14 Changed 8 months ago by rscott

  • Verified working set

This LGTM in ABP/FF 3.5.1.2305. =)

Note: See TracTickets for help on using tickets.