Shift-Click doesn't extend selection in internal PDF plugin

Shift-Click doesn't extend selection in internal PDF plugin

Added code to handle shift and mouse down so that selection will
happen based on first and second click position.

BUG=60344

[Deepak, 2014-11-03 04:44:42]

deepak.m1@samsung.com changed reviewers:
+ raymes@chromium.org

[Deepak, 2014-11-03 04:44:43]

PTAL

[Deepak, 2014-11-05 11:53:42]

On 2014/11/03 04:44:43, Deepak wrote:
> PTAL

PTAL

[Deepak, 2014-11-07 11:21:00]

On 2014/11/05 11:53:42, Deepak wrote:
> On 2014/11/03 04:44:43, Deepak wrote:
> > PTAL

PTAL.

[Deepak, 2014-11-14 06:13:09]

On 2014/11/07 11:21:00, Deepak wrote:
> On 2014/11/05 11:53:42, Deepak wrote:
> > On 2014/11/03 04:44:43, Deepak wrote:
> > > PTAL

@raymes,
PTAL at my changes.

[Deepak, 2014-11-20 02:50:27]

On 2014/11/14 06:13:09, Deepak wrote:
> On 2014/11/07 11:21:00, Deepak wrote:
> > On 2014/11/05 11:53:42, Deepak wrote:
> > > On 2014/11/03 04:44:43, Deepak wrote:
> > > > PTAL
> 
@raymes,
PTAL at my changes.

[Deepak, 2014-12-18 12:40:05]

PTAL.

[Deepak, 2014-12-23 01:14:26]

PTAL.

[Deepak, 2014-12-23 08:54:57]

deepak.m1@samsung.com changed reviewers:
+ gene@chromium.org

[Deepak, 2014-12-24 13:44:38]

@Raymes and @Gene
PTAL.

[Deepak, 2015-01-05 11:14:44]

@Raymes and @Gene
PTAL at my changes.
Thanks

[gene, 2015-01-05 21:55:08]

gene@chromium.org changed reviewers:
+ jam@chromium.org

[gene, 2015-01-05 21:55:36]

+john, he is more familiar with selection code

[Deepak, 2015-01-07 09:21:48]

On 2015/01/05 21:55:36, gene wrote:
> +john, he is more familiar with selection code

@john
PTAL

[Deepak, 2015-01-12 11:39:39]

PTAL at my changes.
Thanks

[jam, 2015-01-12 18:04:27]

https://codereview.chromium.org/697933002/diff/20001/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/697933002/diff/20001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:1662: last_char_index_ = char_index;
why is this set here and in line  3365?

https://codereview.chromium.org/697933002/diff/20001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:3363:
selection_.push_back(PDFiumRange(pages_[page_index], 0, char_index + 1));
why is it 0, char_index+1? normally OnSingleClick would do char_index, 0. i.e.
this behavior change of starting from beginning of page doesn't even happen for
html as far as I can tell

https://codereview.chromium.org/697933002/diff/20001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:3365: last_char_index_ = char_index;
don't we also need to store last_page_index in case the second click was across
a different page?

[Deepak, 2015-01-13 11:04:38]

PTAL.

https://codereview.chromium.org/697933002/diff/20001/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/697933002/diff/20001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:1662: last_char_index_ = char_index;
On 2015/01/12 18:04:27, jam wrote:
> why is this set here and in line  3365?

Done.

https://codereview.chromium.org/697933002/diff/20001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:3363:
selection_.push_back(PDFiumRange(pages_[page_index], 0, char_index + 1));
On 2015/01/12 18:04:27, jam wrote:
> why is it 0, char_index+1? normally OnSingleClick would do char_index, 0. i.e.
> this behavior change of starting from beginning of page doesn't even happen
for
> html as far as I can tell

I have checked as :
http://www.thehindu.com/news/national/pak-supporting-proxy-war-in-jk-army-chi...

First click on the empty space in start of page , 
Then shift click on some text in the main article , selection will happen from
start of the page to the selected word part.

https://codereview.chromium.org/697933002/diff/20001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:3365: last_char_index_ = char_index;
On 2015/01/12 18:04:27, jam wrote:
> don't we also need to store last_page_index in case the second click was
across
> a different page?

Done.

[Deepak, 2015-01-13 11:12:39]

Changes fixes below scenerios:

When we have do operation in same page.

First click	    Second click        Selection Status
                    with Shift   

1.Empty Space	    Empty Space	        No Selection 


2.Empty Space	    Text	        Selection Should happen
                                        from start of the page 
                                        to the Second Click position

3.Text 	            Empty Space	        Selection Should happen from
                                        start of the page to the 
                                        First Click position

4.Text 	            Text	        From First Click position to
                                        the second Click Position.
When mouse click happen across the pages.

First mouse click on some word in page X and second mouse click with
shift pressed,happen on some word in page Y then all the text in 
between these 2 clicks positions should get sselected.

Please correct me if I am missing something.
Thanks

[Deepak, 2015-01-14 12:31:25]

Changes fixes below scenerios:

 When we have do operation in same page.
 
First click	    Second click        Selection Status
                     with Shift   
 
1.Empty Space	    Empty Space	        No Selection 
 
 
2.Empty Space	    Text	        Selection Should happen
                                        from start of the page 
                                        to the Second Click position
 
3.Text 	            Empty Space	        Selection Should happen from
                                        start of the page to the 
                                        First Click position
 
4.Text 	            Text	        From First Click position to
                                        the second Click Position.
When mouse click happen across the pages.
 
First mouse click on some word in page X and second mouse click with
shift pressed,happen on some word in page Y then all the text in 
between these 2 clicks positions should get sselected.
 
Please correct me if I am missing something.
Thanks

[Deepak, 2015-01-15 13:44:58]

@gene
PTAL and share your thoughts.
Thanks

[jam, 2015-01-16 17:21:42]

I patched this and tried it out. some issues:
-I did a normal selection. then I shift clicked after it. the first char of the
selection was lost
-doing this twice in a row (i.e. shift clicking to extend, and then doing it
again) loses selection

[Deepak, 2015-01-17 09:29:33]

On 2015/01/16 17:21:42, jam wrote:
> I patched this and tried it out. some issues:
> -I did a normal selection. then I shift clicked after it. the first char of
the
> selection was lost
> -doing this twice in a row (i.e. shift clicking to extend, and then doing it
> again) loses selection

@Jam, You are right. My explanation for your observations is as:

I am using http://www.selab.isti.cnr.it/ws-mate/example.pdf in my analysis.
1) In chrome we are not getting index for the empty space that is present
between
words.So when we click on space between the words then we either get the
char_index
of previous selectable alphabet of the next selectable alphabet.
and those spaces between the words are not selectable. so

For example:

  "An Example Paper"  that is on First page in above link.

Here we have char_index of 'n' in 'An' word as 151, so when click after 'n' that
is in between 
empty space between 'An' and 'Example' then we are getting either 151 i.e
char_index of 'n' or 
going little more close to 'E' of 'Example' we get 153, i.e char_index of 'E'
So based on where we are doing mouse click we are making selection.

But this is not the case in others
* In adobe reader empty space between the words are selectable and cursor blinks
in PDF.
* In Mozilla FF empty space between words are not selectable and are treated as
char_index -1.
so whole scenario of selection changes.

I have done changes to fix first char selection lost issue.
But as I explained above we need to click close to the next available alphabet.
And selection is mainly depends upon what char_index we are getting.

2) This is intended, as when we do left click then we are clearing all selection
by,

  SelectionChangeInvalidator selection_invalidator(this);
  selection_.clear();

in PDFiumEngine::OnMouseDown().

To clear selection is more  likely as last_char_index and current char_index is
same.
in second case.

PTAL and share your thoughts for this.
Thanks

[jam, 2015-01-23 18:20:39]

On 2015/01/17 09:29:33, Deepak wrote:
> On 2015/01/16 17:21:42, jam wrote:
> > I patched this and tried it out. some issues:
> > -I did a normal selection. then I shift clicked after it. the first char of
> the
> > selection was lost
> > -doing this twice in a row (i.e. shift clicking to extend, and then doing it
> > again) loses selection
> 
> @Jam, You are right. My explanation for your observations is as:
> 
> I am using http://www.selab.isti.cnr.it/ws-mate/example.pdf in my analysis.
> 1) In chrome we are not getting index for the empty space that is present
> between
> words.So when we click on space between the words then we either get the
> char_index
> of previous selectable alphabet of the next selectable alphabet.
> and those spaces between the words are not selectable. so
> 
> For example:
> 
>   "An Example Paper"  that is on First page in above link.
> 
> Here we have char_index of 'n' in 'An' word as 151, so when click after 'n'
that
> is in between 
> empty space between 'An' and 'Example' then we are getting either 151 i.e
> char_index of 'n' or 
> going little more close to 'E' of 'Example' we get 153, i.e char_index of 'E'
> So based on where we are doing mouse click we are making selection.
> 
> But this is not the case in others
> * In adobe reader empty space between the words are selectable and cursor
blinks
> in PDF.
> * In Mozilla FF empty space between words are not selectable and are treated
as
> char_index -1.
> so whole scenario of selection changes.
> 
> I have done changes to fix first char selection lost issue.
> But as I explained above we need to click close to the next available
alphabet.
> And selection is mainly depends upon what char_index we are getting.

I'm clicking in between characters, not in empty space.

> 
> 2) This is intended, as when we do left click then we are clearing all
selection
> by,
> 
>   SelectionChangeInvalidator selection_invalidator(this);
>   selection_.clear();
> 
> in PDFiumEngine::OnMouseDown().
> 
> To clear selection is more  likely as last_char_index and current char_index
is
> same.
> in second case.
> 
> PTAL and share your thoughts for this.

well it seems that you're trying to match what happens with HTML selection. In
which case, we should go all the way and make multiple extensions of the
selection work.
> Thanks

[Deepak, 2015-01-24 11:25:46]

Please refer to table in #18 about expected behavior with this patch.
Let me explain how selection works while clicking in between the characters in 
a word.
Sample String: "ABCDEF XYZ"

So when we want a forward selection, and wanted to select text 
between 'C' and 'Y' included.
Then first click should happen close to start of 'C' so that 
we get proper index for rect of 'C'.
And second click with 'shift' pressed should happen close to end of 'Y'
for getting proper index of 'Y'.

Similarly when when we want a backward selection, and wanted to select text 
between 'C' and 'Y' included.
Then first click should happen close to end of 'Y' so that 
we get proper index for rect of 'Y'.
And second click with 'shift' pressed should happen close to start of 'C'

So the selection depends upon index of the character at click position.

Better way is to do click on the alphabet itself. This will give proper 
rects and works as expected.

I have done few changes.
PTAL and let me know your opinion for this.

Thanks

[Deepak, 2015-01-27 03:32:17]

On 2015/01/24 11:25:46, Deepak wrote:
> Please refer to table in #18 about expected behavior with this patch.
> Let me explain how selection works while clicking in between the characters in

> a word.
> Sample String: "ABCDEF XYZ"
> 
> So when we want a forward selection, and wanted to select text 
> between 'C' and 'Y' included.
> Then first click should happen close to start of 'C' so that 
> we get proper index for rect of 'C'.
> And second click with 'shift' pressed should happen close to end of 'Y'
> for getting proper index of 'Y'.
> 
> Similarly when when we want a backward selection, and wanted to select text 
> between 'C' and 'Y' included.
> Then first click should happen close to end of 'Y' so that 
> we get proper index for rect of 'Y'.
> And second click with 'shift' pressed should happen close to start of 'C'
> 
> So the selection depends upon index of the character at click position.
> 
> Better way is to do click on the alphabet itself. This will give proper 
> rects and works as expected.
> 
> I have done few changes.
> PTAL and let me know your opinion for this.
> 
> Thanks

PTAL

[Deepak, 2015-02-02 13:54:20]

PTAL

[Deepak, 2015-02-09 13:26:08]

PTAL and share your thoughts.
Thanks

[jam, 2015-02-11 20:27:09]

this is still broken per my earlier comment. i'm clicking on text in all cases,
and shift-to-extend only works once and fails again. this is unexpected behavior

i feel that right now we (as devs and reviewers) should be focused on making OOP
ready for all users instead of adding minor features like this. i.e. bug 457457 
is much more important to spend time on.

[Deepak, 2015-02-12 06:12:15]

On 2015/02/11 20:27:09, jam wrote:
> this is still broken per my earlier comment. i'm clicking on text in all
cases,
> and shift-to-extend only works once and fails again. this is unexpected
behavior
> 
> i feel that right now we (as devs and reviewers) should be focused on making
OOP
> ready for all users instead of adding minor features like this. i.e. bug
457457 
> is much more important to spend time on.

ok, I will prepare patch for 457457, And revisit this later.
Thanks