Opened 7 weeks ago

Closed 5 weeks ago

Last modified 4 weeks 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 7 weeks 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 7 weeks 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 7 weeks 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 7 weeks 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 7 weeks 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 7 weeks 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 7 weeks 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 6 weeks ago by kzar

  • Owner set to kzar

comment:10 Changed 6 weeks ago by kzar

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

comment:12 Changed 5 weeks 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 5 weeks ago by kzar

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

comment:14 Changed 4 weeks ago by rscott

  • Verified working set

This LGTM in ABP/FF 3.5.1.2305. =)

Note: See TracTickets for help on using tickets.