Change CBD layout and texts

chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java

Index: chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java b/chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java
new file mode 100644
index 0000000000000000000000000000000000000000..aa3398a87a994e6f4806b85f023072b07bb376f6
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/preferences/ClearBrowsingDataTabCheckBoxPreference.java
@@ -0,0 +1,127 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.preferences;
+
+import android.annotation.SuppressLint;
+import android.content.Context;
+import android.text.SpannableString;
+import android.text.Spanned;
+import android.text.style.ClickableSpan;
+import android.util.AttributeSet;
+import android.view.Gravity;
+import android.view.MotionEvent;
+import android.view.View;
+import android.view.ViewGroup;
+import android.widget.LinearLayout;
+import android.widget.ListView;
+import android.widget.RelativeLayout;
+import android.widget.TextView;
+
+import org.chromium.base.ApiCompatibilityUtils;
+import org.chromium.ui.text.NoUnderlineClickableSpan;
+import org.chromium.ui.text.SpanApplier;
+
+/**
+ * A preference representing one browsing data type in ClearBrowsingDataPreferencesTab.
+ * This class allows clickable links inside the checkbox summary.
+ */
+public class ClearBrowsingDataTabCheckBoxPreference extends ClearBrowsingDataCheckBoxPreference {
+    private Runnable mLinkClickDelegate;
+    private boolean mHasClickableSpans;
+
+    /**
+     * Constructor for inflating from XML.
+     */
+    public ClearBrowsingDataTabCheckBoxPreference(Context context, AttributeSet attrs) {
+        super(context, attrs);
+    }
+
+    /**
+     * @param linkClickDelegate A Runnable that is executed when a link inside the summary is
+     *                          clicked.
+     */
+    public void setLinkClickDelegate(Runnable linkClickDelegate) {
+        mLinkClickDelegate = linkClickDelegate;
+    }
+
+    @Override
+    public View onCreateView(ViewGroup parent) {
+        View view = super.onCreateView(parent);
+
+        final TextView textView = (TextView) view.findViewById(android.R.id.summary);
+
+        // Create custom onTouch listener to be able to respond to click events inside the summary.

[Theresa, 2017/03/24 17:15:37]

Is the desired UX that when you click anywhere besides the ClickableSpan the
preference's checkbox is toggled, and when you click on the ClickableSpan the
link is taken?

I think that's probably not going to work from an accessibility standpoint (see
next comment), because accessibility mode does not give focus to the
ClickableSpan separate from the rest of the summary string. Who are you working
with from UX on this? I'm happy to get looped into a conversation if needed.

[dullweber, 2017/03/27 14:11:04]

My goal was that clicking the text should check or uncheck the checkbox. Only
direct clicks on the myactivity.google.com link should trigger the
LinkClickDelegate. maxwalker@ is doing the UX for this. Discussion about the UI
is on these slides:
https://docs.google.com/a/google.com/presentation/d/1NI9SR2DpFKAsAmvB0-Bn1Qyd....

I tried to use the UI with talkback and I think it would be best if
myactivity.google.com would not be clickable because it is only a small target
and most users would want to check or uncheck the checkbox. 
Users who want to go to their activity can go to myactivity.google.com by
manually entering the address. Is that an acceptable behavior? Otherwise I would
try to use the TextViewWithClickableSpans with a custom layout for the
CheckBoxPreference.

[Theresa, 2017/03/27 15:24:52]

I don't think that is acceptable behavior, but we can always reach out to our
accessibility team for consultation on how this should behave. It's better to
find out now than during launch review once the feature is complete.

Here are a couple of other options.
1) When talk back is off, a direct tap on the URL activates the link and a tap
anywhere else toggles the checkbox. When talk b ack is on, a double tap on any
of the summary text activates the link and taps on the checkbox toggle the
preference.
2) Re-introduce the footer that is in the clear browsing data preference screen
today so that the text view containing the link can be displayed separate from a
preference.

+        textView.setOnTouchListener(new View.OnTouchListener() {
+            @Override
+            @SuppressLint("ClickableViewAccessibility")

[Theresa, 2017/03/24 17:15:37]

We need to  test this with TalkBack on to make sure that the links in the
summary text are clickable when accessibility mode is enabled. Typically, the
expected behavior for text views that contain clickable spans is that the text
is read out when tapped and the clickable span is activated/clicked on double
tap.

This current CBD implementation probably doesn't work with Talkback because
ClickableSpan isn't accessible by default. We have a Chromium subclass,
TextViewWithClickableSpans, that you'll probably need to use:
https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/widg...

You may need to create a custom preference layout that's used for the
preferences with clickable spans in their summary strings. We had to do this for
the existing footers on the CBD preference screen -- see
TextMessageWithLinkAndIconPreference:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

+            public boolean onTouch(View v, MotionEvent event) {
+                if (!mHasClickableSpans) {
+                    return false;
+                }
+                // Find out which character was touched.
+                int offset = textView.getOffsetForPosition(event.getX(), event.getY());
+                // Check if this character contains a span.
+                Spanned text = (Spanned) textView.getText();
+                ClickableSpan[] types = text.getSpans(offset, offset, ClickableSpan.class);
+
+                if (types.length > 0) {
+                    if (event.getAction() == MotionEvent.ACTION_UP) {
+                        for (ClickableSpan type : types) {
+                            type.onClick(textView);
+                        }
+                    }
+                    return true;
+                } else {
+                    return false;
+                }
+            }
+        });
+
+        return view;
+    }
+
+    @Override
+    protected void setupLayout(LinearLayout view) {
+        float density = view.getResources().getDisplayMetrics().density;
+        // Use a custom layout to improve the design with large summary texts.
+        view.setGravity(Gravity.TOP);
+        ListView.LayoutParams lp = (ListView.LayoutParams) view.getLayoutParams();
+        ApiCompatibilityUtils.setPaddingRelative(view, ApiCompatibilityUtils.getPaddingStart(view),
+                (int) (6 * density), ApiCompatibilityUtils.getPaddingEnd(view),

[Theresa, 2017/03/24 17:15:36]

You should define a dimension with a value of "6" in dimens.xml and use
Resources#getDimensionPixelSize() rather than multiplying 6 by the density here.

[dullweber, 2017/03/27 14:11:04]

Done, I also improved the documentation of why I change each part of the layout.

+                (int) (6 * density));
+        view.setLayoutParams(lp);
+
+        RelativeLayout textLayout =
+                (RelativeLayout) view.findViewById(android.R.id.title).getParent();
+        ApiCompatibilityUtils.setPaddingRelative(textLayout,
+                ApiCompatibilityUtils.getPaddingStart(textLayout), 0,
+                ApiCompatibilityUtils.getPaddingEnd(textLayout), 0);

[Theresa, 2017/03/24 17:15:37]

If possible, it is highly preferable to avoid this custom padding because it
adds additional engineering overhead and maintenance, particularly when we go
through another style redesign (happens every few years). We have a broad team
effort to make our UI more consistent to reduce the burden on UX and eng and
make the product feel more consistent overall.

We use checkbox preferences with longer summary strings in our "Privacy"
settings screen. Is there a reason that the padding here needs to be different
than it is there? What does it look like if we don't adjust the padding and
gravity?

[dullweber, 2017/03/27 14:11:04]

I made these layout changes to align the checkbox with the top of the title
instead of the center of the component. Max asked me to this because the summary
text can be 
It is documented in the specs that the checkbox should be 16dp from the top and
right, independent of the size of the summary text. 
https://screenshot.googleplex.com/HazfXCSSL7p.png here is a screenshot with the
padding and Gravity.TOP disabled. I noticed that the first Gravity.TOP entry was
useless and removed it. Only the gravity on the widget_frame is needed to pull
the checkbox up

[Theresa, 2017/03/27 15:24:52]

The UX decision either needs to be consistent with our existing preference
styles or apply to all preferences (ideally they would all change at the same
time). It's part of our job as eng to enforce consistency when the design
guideline differs from what is implemented in other parts of the product.

[dullweber, 2017/03/30 09:11:58]

I removed the custom gravity on the checkbox. This way we can submit this cl and
talk about the checkbox ui and accessibility later.

+
+        LinearLayout widgetFrame = (LinearLayout) view.findViewById(android.R.id.widget_frame);
+        widgetFrame.setGravity(Gravity.TOP);
+    }
+
+    @Override
+    public void setSummary(CharSequence summary) {
+        // If there is no link in the summary, invoke the default behavior.
+        String summaryString = summary.toString();
+        if (!summaryString.contains("<link>") || !summaryString.contains("</link>")) {
+            super.setSummary(summary);
+            return;
+        }
+
+        // Linkify <link></link> span.
+        final SpannableString summaryWithLink = SpanApplier.applySpans(summaryString,
+                new SpanApplier.SpanInfo("<link>", "</link>", new NoUnderlineClickableSpan() {
+                    @Override
+                    public void onClick(View widget) {
+                        if (mLinkClickDelegate != null) mLinkClickDelegate.run();
+                    }
+                }));
+
+        mHasClickableSpans = true;
+        super.setSummary(summaryWithLink);
+    }
+}