Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2963753003:
Fix discrepancies in form text selection between PDF and HTML forms. (Closed)
DescriptionFix discrepancies in form text selection between PDF and HTML forms.
Refactored form text selection code in pdfium_engine.cc so plugin text
selection changes whenever a new character is selected/unselected.
Previously, only sent updates after a user released the left mouse
button or released the shift key.
This change also fixes the following issue: if a user selected text with
the mouse and released the left mouse button outside the form text area,
the plugin text selection would not be set and text could not be copied.
BUG=59266
Review-Url: https://codereview.chromium.org/2963753003
Cr-Commit-Position: refs/heads/master@{#484732}
Committed: https://chromium.googlesource.com/chromium/src/+/b0175c42eca66d5bad2e7d41d2fc125e78b03122
Patch Set 1 #Patch Set 2 : Remove extra parentheses #
Total comments: 8
Patch Set 3 : Remove |old_selected_text_| #Patch Set 4 : Rebase #Patch Set 5 : Remove unneeded file #
Total comments: 10
Patch Set 6 : Rename and relocate SetMouseHeldDown() to indicate left button #
Total comments: 2
Patch Set 7 : Adjust calls to SetMouseLeftButtonDown() to avoid early returns #
Total comments: 4
Patch Set 8 : Initialize in_form_text_area_ and mouse_left_button_down_ + change comment in SetFormSelectedText #
Dependent Patchsets: Messages
Total messages: 23 (9 generated)
Description was changed from ========== Fix discrepancies in form text selection between PDF and HTML forms. Refactored form text selection code in pdfium_engine.cc so plugin text selection changes whenever a new character is selected/unselected. Previously, only sent updates after a user released the left mouse button or released the shift key. This change also fixes the following issue: if a user selected text with the mouse and released the left mouse button outside the form text area, the plugin text selection would not be set and text could not be copied. BUG=59266 ========== to ========== Fix discrepancies in form text selection between PDF and HTML forms. Refactored form text selection code in pdfium_engine.cc so plugin text selection changes whenever a new character is selected/unselected. Previously, only sent updates after a user released the left mouse button or released the shift key. This change also fixes the following issue: if a user selected text with the mouse and released the left mouse button outside the form text area, the plugin text selection would not be set and text could not be copied. BUG=59266 ==========
drgage@google.com changed reviewers: + dsinclair@chromium.org, thestig@chromium.org
Description was changed from ========== Fix discrepancies in form text selection between PDF and HTML forms. Refactored form text selection code in pdfium_engine.cc so plugin text selection changes whenever a new character is selected/unselected. Previously, only sent updates after a user released the left mouse button or released the shift key. This change also fixes the following issue: if a user selected text with the mouse and released the left mouse button outside the form text area, the plugin text selection would not be set and text could not be copied. BUG=59266 ========== to ========== Fix discrepancies in form text selection between PDF and HTML forms. Refactored form text selection code in pdfium_engine.cc so plugin text selection changes whenever a new character is selected/unselected. Previously, only sent updates after a user released the left mouse button or released the shift key. This change also fixes the following issue: if a user selected text with the mouse and released the left mouse button outside the form text area, the plugin text selection would not be set and text could not be copied. BUG=59266 ==========
drgage@google.com changed reviewers: + dsinclair@chromium.org, thestig@chromium.org
Hi Lei and Dan, In this CL, I have changed the form text selection code in pdfium_engine.cc so the selection is updated after each new character is highlighted/unhighlighted. To do this, I am now keeping track of current and previous selected text strings and comparing them to determine if the plugin's selected text should be updated. This also led me to remove the check in PDFiumEngine::OnKeyDown() for if the plugin text selection should be cleared; this is taken care of with the new logic. Thanks for reviewing. Best, Diana
https://codereview.chromium.org/2963753003/diff/20001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (left): https://codereview.chromium.org/2963753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1860: if (area == PDFiumPage::FORM_TEXT_AREA) This change is a bit out of date and won't apply anymore. https://codereview.chromium.org/2963753003/diff/20001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2963753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1650: // Update previous and current selections, then compare them to check if If this method does not have |old_selected_form_text_|, and this code just became: std::string selected_form_text = base::UTF16ToUTF8(selected_form_text16); if (selected_form_text != selected_form_text_) { selected_form_text_ = selected_form_text; pp::PDF::SetSelectedText(GetPluginInstance(), selected_form_text_.c_str()); } Would that work? Can you explain what purpose |old_selected_form_text_| serves? https://codereview.chromium.org/2963753003/diff/20001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2963753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:673: bool mouse_held_down_; Do we need |mouse_held_down_| if we already have |selecting_| on line 659 above?
Hi Lei and Dan, In this patchset, I have removed the |old_selected_text_| member variable in PDFiumEngine, and instead am using a local variable in the function PDFiumEngine::SetFormSelectedText(). Thanks for reviewing. Diana https://codereview.chromium.org/2963753003/diff/20001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (left): https://codereview.chromium.org/2963753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1860: if (area == PDFiumPage::FORM_TEXT_AREA) On 2017/06/29 02:52:46, Lei Zhang wrote: > This change is a bit out of date and won't apply anymore. I'm not sure what you mean. I removed this check because it doesn't apply anymore. https://codereview.chromium.org/2963753003/diff/20001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2963753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1650: // Update previous and current selections, then compare them to check if On 2017/06/29 02:52:46, Lei Zhang wrote: > If this method does not have |old_selected_form_text_|, and this code just > became: > > std::string selected_form_text = base::UTF16ToUTF8(selected_form_text16); > if (selected_form_text != selected_form_text_) { > selected_form_text_ = selected_form_text; > pp::PDF::SetSelectedText(GetPluginInstance(), selected_form_text_.c_str()); > } > > Would that work? Can you explain what purpose |old_selected_form_text_| serves? Yes, something like that would work. That's actually what I had first, but I changed it so that |old_selected_text_| was a member variable because I was thinking it was necessary for making the check on line 1641 and exiting early if possible. Looking at it again today, I see that it's not necessary and the check can be made without it. https://codereview.chromium.org/2963753003/diff/20001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2963753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:673: bool mouse_held_down_; On 2017/06/29 02:52:46, Lei Zhang wrote: > Do we need |mouse_held_down_| if we already have |selecting_| on line 659 above? The reason I added |mouse_held_down_| is because the way |selecting_| is currently being used is for text selection only (i.e. it corresponds to the |selection_| variable). If you see how it is used in pdfium_engine.cc, it's set to true in PDFiumEngine::OnSingleClick(), which is only called in PDFiumEngine::OnMouseDown() in PDFiumPage::TEXT_AREAs. Also, much of what needs to happen for form text area selection in PDFiumEngine::OnMouseMove() can only happen if |selecting_| is set to false. If you think it would make more sense to try to change the way |selecting_| is being used as opposed to adding another variable, I can look into doing that.
https://codereview.chromium.org/2963753003/diff/20001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (left): https://codereview.chromium.org/2963753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1860: if (area == PDFiumPage::FORM_TEXT_AREA) On 2017/06/29 17:49:58, drgage wrote: > On 2017/06/29 02:52:46, Lei Zhang wrote: > > This change is a bit out of date and won't apply anymore. > > I'm not sure what you mean. I removed this check because it doesn't apply > anymore. Didn't you already change this block of code to also check: last_page_mouse_down_ != -1 ? That change needs to be reflected in the diff, for it to apply correctly. i.e. your local git branch associated with this CL likely does not include https://crbug.com/483133 .
Hi Lei and Dan, I have rebased so that the other bug fix is included in the git history for this branch. Thanks for reviewing. Diana https://codereview.chromium.org/2963753003/diff/20001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (left): https://codereview.chromium.org/2963753003/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1860: if (area == PDFiumPage::FORM_TEXT_AREA) On 2017/06/29 22:38:04, Lei Zhang wrote: > On 2017/06/29 17:49:58, drgage wrote: > > On 2017/06/29 02:52:46, Lei Zhang wrote: > > > This change is a bit out of date and won't apply anymore. > > > > I'm not sure what you mean. I removed this check because it doesn't apply > > anymore. > > Didn't you already change this block of code to also check: > last_page_mouse_down_ != -1 ? That change needs to be reflected in the diff, for > it to apply correctly. i.e. your local git branch associated with this CL likely > does not include https://crbug.com/483133 . Oh yeah, at the time that I started working on this I had checked out this branch from master, not the branch where I fixed that. But now since master includes that change, I'll rebase and upload a new patchset.
https://codereview.chromium.org/2963753003/diff/80001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2963753003/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1734: SetMouseHeldDown(true); If SetMouseHeldDown() is really SetMouseLeftButtonDown(), then this isn't 100% correct since code executation can also reach this point for a middle click. Same issue with the call on line 1826. https://codereview.chromium.org/2963753003/diff/80001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2963753003/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:457: void SetMouseHeldDown(bool mouse_held_down); How about SetMouseLeftButtonDown(bool is_mouse_left_button_down) ? https://codereview.chromium.org/2963753003/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:665: // Used for selection of text within form text areas (text fields and combo Just: The currently selected text within form ... https://codereview.chromium.org/2963753003/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:673: bool mouse_held_down_; Also rename to indicate it's the left mouse button. https://codereview.chromium.org/2963753003/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:675: MouseDownState mouse_down_state_; Let's move this up to be with |selecting_|, so the non-form text selection variables are grouped together.
Patchset #6 (id:100001) has been deleted
Hi Lei and Dan, In this patchset, I have renamed |mouse_held_down_| and SetMouseHeldDown() so they correspond to the left mouse button being down. I have also moved the function call so that it only occurs when the left mouse button is pressed. Thanks for reviewing. Best, Diana https://codereview.chromium.org/2963753003/diff/80001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2963753003/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1734: SetMouseHeldDown(true); On 2017/06/30 21:39:36, Lei Zhang wrote: > If SetMouseHeldDown() is really SetMouseLeftButtonDown(), then this isn't 100% > correct since code executation can also reach this point for a middle click. > Same issue with the call on line 1826. Done. https://codereview.chromium.org/2963753003/diff/80001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2963753003/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:457: void SetMouseHeldDown(bool mouse_held_down); On 2017/06/30 21:39:36, Lei Zhang wrote: > How about SetMouseLeftButtonDown(bool is_mouse_left_button_down) ? Done. https://codereview.chromium.org/2963753003/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:665: // Used for selection of text within form text areas (text fields and combo On 2017/06/30 21:39:36, Lei Zhang wrote: > Just: The currently selected text within form ... Done. https://codereview.chromium.org/2963753003/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:673: bool mouse_held_down_; On 2017/06/30 21:39:36, Lei Zhang wrote: > Also rename to indicate it's the left mouse button. Done. https://codereview.chromium.org/2963753003/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:675: MouseDownState mouse_down_state_; On 2017/06/30 21:39:36, Lei Zhang wrote: > Let's move this up to be with |selecting_|, so the non-form text selection > variables are grouped together. Done.
https://codereview.chromium.org/2963753003/diff/120001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2963753003/diff/120001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1751: SetMouseLeftButtonDown(true); So if we move the call here, then if the user left clicks (and holds down the mouse button) on a link, the code would return early at line 1737. Similarly, if the user releases the mouse button on a link, SetMouseLeftButtonDown() also does not get called in OnMouseUp(). Can we either fix the comments for SetMouseLeftButtonDown() / |mouse_left_button_down_| to account for this, or adjust the calls to SetMouseLeftButtonDown() so they do what the comments say they do?
Hi Lei and Dan, In this patchset, I moved the calls to SetMouseLeftButtonDown() further up in PDFiumEngine::OnMouseUp() and PDFiumEngine::OnMouseDown(). With this change, the |mouse_left_button_down_| is set before early returns in the functions that check for middle mouse button events and whether or not the area is a link. Thanks for reviewing. Diana https://codereview.chromium.org/2963753003/diff/120001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2963753003/diff/120001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1751: SetMouseLeftButtonDown(true); On 2017/07/01 00:30:46, Lei Zhang wrote: > So if we move the call here, then if the user left clicks (and holds down the > mouse button) on a link, the code would return early at line 1737. Similarly, if > the user releases the mouse button on a link, SetMouseLeftButtonDown() also does > not get called in OnMouseUp(). > > Can we either fix the comments for SetMouseLeftButtonDown() / > |mouse_left_button_down_| to account for this, or adjust the calls to > SetMouseLeftButtonDown() so they do what the comments say they do? Done.
lgtm https://codereview.chromium.org/2963753003/diff/140001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2963753003/diff/140001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1640: // NUL-terminator. Update the comment, now that the if-statement has changed?
https://codereview.chromium.org/2963753003/diff/140001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2963753003/diff/140001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:674: bool mouse_left_button_down_; We need to initialize this. We also forgot to initialize |in_form_text_area_| in https://codereview.chromium.org/2924343005/
Hi Lei and Dan, In this patchset, I addressed Lei's comment regarding initializing the boolean variables I added in PDFiumEngine and changing the wording of the comment in SetFormSelectedText(). Thanks for reviewing. Diana https://codereview.chromium.org/2963753003/diff/140001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2963753003/diff/140001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1640: // NUL-terminator. On 2017/07/05 19:31:12, Lei Zhang wrote: > Update the comment, now that the if-statement has changed? Done. https://codereview.chromium.org/2963753003/diff/140001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2963753003/diff/140001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.h:674: bool mouse_left_button_down_; On 2017/07/05 22:02:21, Lei Zhang wrote: > We need to initialize this. We also forgot to initialize |in_form_text_area_| in > https://codereview.chromium.org/2924343005/ Done.
The CQ bit was checked by drgage@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2963753003/#ps160001 (title: "Initialize in_form_text_area_ and mouse_left_button_down_ + change comment in SetFormSelectedText")
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": 160001, "attempt_start_ts": 1499368635737290,
"parent_rev": "39446b44eebc2a55f4a1192fd83b27181a5a17f3", "commit_rev":
"b0175c42eca66d5bad2e7d41d2fc125e78b03122"}
Message was sent while issue was closed.
Description was changed from ========== Fix discrepancies in form text selection between PDF and HTML forms. Refactored form text selection code in pdfium_engine.cc so plugin text selection changes whenever a new character is selected/unselected. Previously, only sent updates after a user released the left mouse button or released the shift key. This change also fixes the following issue: if a user selected text with the mouse and released the left mouse button outside the form text area, the plugin text selection would not be set and text could not be copied. BUG=59266 ========== to ========== Fix discrepancies in form text selection between PDF and HTML forms. Refactored form text selection code in pdfium_engine.cc so plugin text selection changes whenever a new character is selected/unselected. Previously, only sent updates after a user released the left mouse button or released the shift key. This change also fixes the following issue: if a user selected text with the mouse and released the left mouse button outside the form text area, the plugin text selection would not be set and text could not be copied. BUG=59266 Review-Url: https://codereview.chromium.org/2963753003 Cr-Commit-Position: refs/heads/master@{#484732} Committed: https://chromium.googlesource.com/chromium/src/+/b0175c42eca66d5bad2e7d41d2fc... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/b0175c42eca66d5bad2e7d41d2fc... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
