Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2945703002:
add support for rich text selections (Closed)
|
Created:
3 years, 6 months ago by David Tseng Modified:
3 years, 6 months ago Reviewers:
dmazzoni CC:
chromium-reviews, alemate+watch_chromium.org, dtseng+watch_chromium.org, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, dougt+watch_chromium.org, dmazzoni+watch_chromium.org, oshima+watch_chromium.org, aleventhal+watch_chromium.org, je_julie, arv+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionadd support for rich text selections
BUG=719654
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2945703002
Cr-Commit-Position: refs/heads/master@{#481283}
Committed: https://chromium.googlesource.com/chromium/src/+/f89d3572342f8a1bad997cec5fbabcdfe33aa5ef
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address comments. #Patch Set 3 : Account for lines without any text #Patch Set 4 : Rebase. #
Total comments: 11
Patch Set 5 : Address comments. #
Messages
Total messages: 31 (24 generated)
Description was changed from ========== WIP: add support for rich text selections BUG= ========== to ========== WIP: add support for rich text selections BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dtseng@chromium.org changed reviewers: + dmazzoni@chromium.org
PTAL. Ready for an initial look. Things just about work everywhere.
https://codereview.chromium.org/2945703002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js (right): https://codereview.chromium.org/2945703002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:230: this.anchorLine_ = new editing.EditableLine(root.anchorObject, Is the plan to refactor EditableLine to only take two arguments now? https://codereview.chromium.org/2945703002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:340: // Describe a generic DOM selection. Want to put some sort of placeholder here? Or maybe just favor the change to the focus?
Description was changed from ========== WIP: add support for rich text selections BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== add support for rich text selections BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
PTAL; added tests and removed WIP. https://codereview.chromium.org/2945703002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js (right): https://codereview.chromium.org/2945703002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:230: this.anchorLine_ = new editing.EditableLine(root.anchorObject, On 2017/06/19 15:11:51, dmazzoni wrote: > Is the plan to refactor EditableLine to only take two arguments now? No, added comments to clarify. An editable line contains both the selection state and the line state. For things larger than a line, we want to truncate the selection at bounds. We're saving anchor and focus line to get the selection edges to do state comparisons (as to avoi generating all lines in the selection). https://codereview.chromium.org/2945703002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:340: // Describe a generic DOM selection. On 2017/06/19 15:11:51, dmazzoni wrote: > Want to put some sort of placeholder here? > > Or maybe just favor the change to the focus? Removed. We don't want to favor focus but favor the line edge of the selection that changed which is actually |cur|. I added more comments and tests to try to clarify.
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2945703002/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js (right): https://codereview.chromium.org/2945703002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:724: * @return {boolean} Add @param {editing.EditableLine}, I'm assuming a self-reference is allowed https://codereview.chromium.org/2945703002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:726: equalsStart: function(otherLine) { how about hasSameStart? https://codereview.chromium.org/2945703002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:747: equals: function(otherLine) { I think it'd be more clear to rename equalsStrict to equals, and call this something like spansSameLines or something like that https://codereview.chromium.org/2945703002/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/editing_test.extjs (right): https://codereview.chromium.org/2945703002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/editing_test.extjs:633: // Notice that the end offset is properly retained. nit: indent https://codereview.chromium.org/2945703002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/editing_test.extjs:640: // Notice that the end offset is truncated up to the end of line. nit: indent https://codereview.chromium.org/2945703002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/editing_test.extjs:643: // Across paragraph dselection with base line on focus. nit: dselection -> selection, and again below
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Description was changed from ========== add support for rich text selections BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== add support for rich text selections BUG=719654 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2945703002/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js (right): https://codereview.chromium.org/2945703002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:724: * @return {boolean} On 2017/06/21 06:22:07, dmazzoni wrote: > Add @param {editing.EditableLine}, I'm assuming a self-reference is allowed Acknowledged and done elsewhere for isSameLine and isSameLineAndSelection (I renamed those for clarity as requested; not seeing those comments now that were in email only). https://codereview.chromium.org/2945703002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js:726: equalsStart: function(otherLine) { On 2017/06/21 06:22:08, dmazzoni wrote: > how about hasSameStart? Start is really overloaded in all of the editing code; it would be more like hasSameLineStart or something. I removed the two methods to reduce the complexity of the public interface. https://codereview.chromium.org/2945703002/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/editing_test.extjs (right): https://codereview.chromium.org/2945703002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/editing_test.extjs:633: // Notice that the end offset is properly retained. On 2017/06/21 06:22:08, dmazzoni wrote: > nit: indent Done. https://codereview.chromium.org/2945703002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/editing_test.extjs:640: // Notice that the end offset is truncated up to the end of line. On 2017/06/21 06:22:08, dmazzoni wrote: > nit: indent Done (hmm git cl format didn't catch this). https://codereview.chromium.org/2945703002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/editing_test.extjs:643: // Across paragraph dselection with base line on focus. On 2017/06/21 06:22:08, dmazzoni wrote: > nit: dselection -> selection, and again below Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dtseng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2945703002/#ps80001 (title: "Address comments.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498074646221530,
"parent_rev": "f072428c5dbc5d7a1b5a40afe521324c2a08c7f3", "commit_rev":
"f89d3572342f8a1bad997cec5fbabcdfe33aa5ef"}
Message was sent while issue was closed.
Description was changed from ========== add support for rich text selections BUG=719654 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== add support for rich text selections BUG=719654 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2945703002 Cr-Commit-Position: refs/heads/master@{#481283} Committed: https://chromium.googlesource.com/chromium/src/+/f89d3572342f8a1bad997cec5fba... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/f89d3572342f8a1bad997cec5fba... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
