Closed Bug 1463576 Opened Last year Closed Last year

Write tests for SelectionActionDelegate

Categories

(GeckoView :: General, enhancement, P2)

All
Android
enhancement

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(8 files)

SelectionActionDelegate needs some tests.
Comment on attachment 8981573 [details]
Bug 1463576 - 1. Add layout.accessiblecaret.script_change_update_mode pref;

https://reviewboard.mozilla.org/r/247666/#review253750

::: layout/base/AccessibleCaretManager.cpp:111
(Diff revision 1)
>                                   "layout.accessiblecaret.bar.enabled");
>      Preferences::AddBoolVarCache(&sCaretShownWhenLongTappingOnEmptyContent,
>        "layout.accessiblecaret.caret_shown_when_long_tapping_on_empty_content");
>      Preferences::AddBoolVarCache(&sCaretsAlwaysTilt,
>                                   "layout.accessiblecaret.always_tilt");
> -    Preferences::AddBoolVarCache(&sCaretsScriptUpdates,
> +    Preferences::AddIntVarCache(&sCaretsScriptUpdates,

This should ideally switch to staticprefs...  Followup is probably ok.

::: mobile/android/app/mobile.js:786
(Diff revision 1)
>  // Androids carets are always tilt to match the text selection guideline.
>  pref("layout.accessiblecaret.always_tilt", true);
>  
> -// Selection change notifications generated by Javascript changes
> -// update active AccessibleCarets / UI interactions.
> -pref("layout.accessiblecaret.allow_script_change_updates", true);
> +// Update any visible carets for selection changes due to JS calls,
> +// but don't show carets if carets are hidden.
> +pref("layout.accessiblecaret.script_change_update_mode", 1);

Is this a pref we expect users to have been setting?  If so, how will this change interact with user-set values?

Or was this just a hook for Fennec to customize behavior?
Attachment #8981573 - Flags: review?(bzbarsky) → review+
Comment on attachment 8981573 [details]
Bug 1463576 - 1. Add layout.accessiblecaret.script_change_update_mode pref;

https://reviewboard.mozilla.org/r/247666/#review253750

> Is this a pref we expect users to have been setting?  If so, how will this change interact with user-set values?
> 
> Or was this just a hook for Fennec to customize behavior?

It was just a hook for Fennec.
Comment on attachment 8981575 [details]
Bug 1463576 - 3. Add external delegate support in GeckoSessionTestRule;

https://reviewboard.mozilla.org/r/247670/#review254508

Wow, this is a lot to ingest, but lgtm!
Attachment #8981575 - Flags: review?(snorp) → review+
Comment on attachment 8981576 [details]
Bug 1463576 - 4. Add SelectionActionDelegateTest;

https://reviewboard.mozilla.org/r/247672/#review254510
Attachment #8981576 - Flags: review?(snorp) → review+
Comment on attachment 8981577 [details]
Bug 1463576 - 5. Use docshell commands for selection actions;

https://reviewboard.mozilla.org/r/247674/#review254514
Attachment #8981577 - Flags: review?(snorp) → review+
Comment on attachment 8981579 [details]
Bug 1463576 - 7. Simplify clipboard code;

https://reviewboard.mozilla.org/r/247678/#review254516

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/Clipboard.java:25
(Diff revision 1)
>      private Clipboard() {
>      }
>  
>      @WrapForJNI(calledFrom = "gecko")
>      public static String getText(final Context context) {
> -        // If we're on the UI thread or the background thread, we have a looper on the thread
> +        final ClipboardManager cm = (ClipboardManager)

I guess all of this is ok from the gecko thread?
Attachment #8981579 - Flags: review?(snorp) → review+
Comment on attachment 8981580 [details]
Bug 1463576 - 8. Hide selection actions on pagehide;

https://reviewboard.mozilla.org/r/247680/#review254518
Attachment #8981580 - Flags: review?(snorp) → review+
Comment on attachment 8981574 [details]
Bug 1463576 - 2. Add test-only geckoview.selection_action.show_on_focus pref;

https://reviewboard.mozilla.org/r/247668/#review254768
Attachment #8981574 - Flags: review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 8f92398ac06af2fe9b6b05b72072e24d2703d124 -d 4a8ac9618893: rebasing 466454:8f92398ac06a "Bug 1463576 - 1. Add layout.accessiblecaret.script_change_update_mode pref; r=bz"
merging layout/base/AccessibleCaretManager.cpp
merging modules/libpref/init/all.js
rebasing 466455:1f6b1a3c01f0 "Bug 1463576 - 2. Add test-only geckoview.selection_action.show_on_focus pref; r=jchen"
rebasing 466456:f579f9dc0e2e "Bug 1463576 - 3. Add external delegate support in GeckoSessionTestRule; r=snorp"
rebasing 466457:871973fbca0c "Bug 1463576 - 4. Add SelectionActionDelegateTest; r=snorp"
merging mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/BaseSessionTest.kt
warning: conflicts while merging mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/BaseSessionTest.kt! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92022a209721
1. Add layout.accessiblecaret.script_change_update_mode pref; r=bz
https://hg.mozilla.org/integration/autoland/rev/fb9bb29f0b40
2. Add test-only geckoview.selection_action.show_on_focus pref; r=jchen
https://hg.mozilla.org/integration/autoland/rev/68ddcec7bb45
3. Add external delegate support in GeckoSessionTestRule; r=snorp
https://hg.mozilla.org/integration/autoland/rev/a3eab1ac5c28
4. Add SelectionActionDelegateTest; r=snorp
https://hg.mozilla.org/integration/autoland/rev/f30ca96560d7
5. Use docshell commands for selection actions; r=snorp
https://hg.mozilla.org/integration/autoland/rev/8f4c34fd430f
6. Refactor GeckoRuntime; r=jchen
https://hg.mozilla.org/integration/autoland/rev/3f54e4438953
7. Simplify clipboard code; r=snorp
https://hg.mozilla.org/integration/autoland/rev/8dae9b53c356
8. Hide selection actions on pagehide; r=snorp
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 62 → mozilla62
You need to log in before you can comment on or make changes to this bug.