Bug 255 - making a selection in url field while vkb is hidden triggers vkb which selects the entire string
: making a selection in url field while vkb is hidden triggers vkb which select...
Status: RESOLVED FIXED
Product: Browser
User interface
: 1.0
: All All
: Medium enhancement (vote)
: 4.0
Assigned To: timeless
: browser-bugs
:
: enhancement-it2005
:
:
  Show dependency tree
 
Reported: 2005-11-11 14:16 UTC by Niels Breet
Modified: 2007-10-27 20:33 UTC (History)
0 users (show)

See Also:


Attachments
only select the whole address field if the user hasn't made a selection (647 bytes, patch)
2007-08-30 09:02 UTC, timeless
Details


Note

You need to log in before you can comment on or make changes to this bug.


Description Niels Breet (reporter) maemo.org 2005-11-11 14:16:32 UTC
When the browser is in fullscreen mode, it displays the url field in the lower
left corner. If you select some part of the url there, part of the text gets
selected. The keyboard then pops up and displays the url which is now fully
selected. This gets me a lot when changing for example planet.maemo.org to
maemo.org while in fullscreen mode.

Same thing the other way around. Select a part of the url while the keyboard is
on screen. Now close the keyboard. The url is deselected.
Comment 1 Maemo QA (deprecated) 2006-04-27 13:11:49 UTC
Claming ownership.
Comment 2 Maemo QA (deprecated) 2006-04-27 13:13:49 UTC
Setting severity to enhancement.
Comment 3 Maemo QA (deprecated) 2006-05-22 14:10:27 UTC
Feature request has been forwarded to upstream maintainer.
Comment 4 timeless 2007-08-30 08:51:56 UTC
> Select a part of the url while the keyboard is on screen.
> Now close the keyboard. The url is deselected.

This seems to work for me.
I'm using:

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9a6pre) Gecko/20070810
Firefox/3.0a1 Tablet browser 0.1.16 RX-34_2007SE_4.2007.26-4

which is basically 4.2007 with the second microb bundle.

In my testing, whether I'm using full screen or not, the same behavior happens.

A quick snoop:
http://timeless.justdave.net/mxr-test/garage/source/browser/
- Welcome to a slightly dated version of the source base (it's indexed, and
linkified, it's also on a server which is currently straining because of the
total space [~60GB] used by all the referenced bits, which means it might be
down for a while, if it is, remember, it's a free service, you aren't paying
for it [in fact, a friend is giving me the service]).

[dir]    browser-ui/     -     Jul 27 15:14      
[dir]    mozilla/     -     Jul 27 15:14      
[lnk]    www/     -     Aug 10 09:38       

- we probably want browser-ui, click.
http://timeless.justdave.net/mxr-test/garage/source/browser/browser-ui/


[lnk]    trunk/     -     Jul 12 14:34       

-click.
http://timeless.justdave.net/mxr-test/garage/source/browser/browser-ui/trunk/

[dir]    browser-eal/     -     Aug 10 09:39     Web Engine Abstraction Layer.
[dir]    dummy-eal/     -     Jul 12 14:34     Dummy Web Engine.
[dir]    eal-test/     -     Jul 12 14:34      
[dir]    engine-chooser/     -     Jul 12 14:34     Launcher of browser ui with
different engines
[dir]    tablet-browser-interface/     -     Aug 23 03:26     Browser, meta
package for browser
[dir]    tablet-browser-translations-dev/     -     Jul 12 14:34     <insert up
to 60 chars description>
[dir]    tablet-browser-ui/     -     Jul 12 14:34     Browser for embedded
platforms 

- that last one looks good, click.
    Application
- not a very useful description, it comes from:
http://timeless.justdave.net/mxr-test/garage/source/browser/browser-ui/trunk/tablet-browser-ui/debian/control#12
but it's actually not wrong, browser-ui really is the application. We're
accepting patches if you want to improve this :).

      Name     Size     Date (GMT)     Description
[up.]    Parent directory     -     Jul 12 14:34      
[dir]    data/     -     Jul 12 14:34      
- you'd have to look into this to see what it is
[dir]    debian/     -     Jul 12 14:34     
- debian directory, one bit has already been noted about it. 
[dir]    include/     -     Jul 12 14:34      
- typical include directory
[dir]    src/     -     Jul 12 14:34       
- sources

click.
http://timeless.justdave.net/mxr-test/garage/source/browser/browser-ui/trunk/tablet-browser-ui/src/

<Long list...>, well, you said toolbar, so:

[fil]    browsertoolbar.c     11k     Jul 12 14:34      
[fil]    browsertoolbar_cb.c     21k     Jul 12 14:34       

We can pick either, _cb happens to mean callback, so I clicked it (and was
right). But let's pretend I didn't know.

click the first one:

two magic words come to mind "focus" and "address", they both turn up in the
file:
 227     gtk_widget_grab_focus(GTK_WIDGET(self->address_combo));

We could follow the address_combo link, it should work.

But I chose to read and saw this:
 123 browser_toolbar_init (BrowserToolbar *self)
...
 136     toolbar_connect_signals(self);
-since the file starts out talking a lot about signals, i clicked:
http://timeless.justdave.net/mxr-test/garage/ident?i=toolbar_connect_signals

Defined as a function prototype in:

    * browser/browser-ui/trunk/tablet-browser-ui/include/browsertoolbar_cb.h
          o line 31 
-header file, gives a hint that the implementation is probably in a similarly
named file
    * browser/browser-ui/trunk/tablet-browser-ui/src/browsertoolbar.c
          o line 136 
-we came from here, no point in going back

Referenced in:
    * browser/browser-ui/trunk/tablet-browser-ui/src/browsertoolbar_cb.c
          o line 475 
-process of elimination says, to go here, so does the header hint.
-click
http://timeless.justdave.net/mxr-test/garage/source/browser/browser-ui/trunk/tablet-browser-ui/src/browsertoolbar_cb.c#475

 475 void toolbar_connect_signals (BrowserToolbar *self)
 476 {
...
 480     g_signal_connect(GTK_WIDGET(self->address_entry),
 481                      "button_release_event",
 482                      G_CALLBACK(url_entry_focus_in_event_cb),
 483                      self);

Well, that sounds promising, it has "address_entry", and "button" and "focus",
all the magic words we could possibly want.

-click that callback bit,
http://timeless.justdave.net/mxr-test/garage/ident?i=url_entry_focus_in_event_cb

Defined as a function in:
    * browser/browser-ui/trunk/tablet-browser-ui/src/browsertoolbar_cb.c
          o line 82 
-well, this is clearly what we want

Defined as a variable in:
    * browser/browser-ui/trunk/tablet-browser-ui/src/browsertoolbar_cb.c
          o line 482 
-we are here

Defined as a function prototype in:
    * browser/browser-ui/trunk/tablet-browser-ui/include/browsertoolbar_cb.h
          o line 36 
-header file, hint

Referenced in: 
-nowhere, kinda silly isn't it? well, someday I'll fix this line to not appear.

-click 82.
http://timeless.justdave.net/mxr-test/garage/source/browser/browser-ui/trunk/tablet-browser-ui/src/browsertoolbar_cb.c#82

  82 gboolean url_entry_focus_in_event_cb(GtkWidget * widget,
  83                                      GdkEventButton * event,
  84                                      BrowserToolbar *self)
-we're getting closer, less eliding
  85 {
  86     ULOG_DEBUG_F();
-debugging. you could click
Limit output to pattern: h$ research
http://timeless.justdave.net/mxr-test/garage/ident?i=ULOG_DEBUG_F&tree=garage&filter=h%24 
* browser/browser-ui/trunk/tablet-browser-ui/include/common.h
 * line 48 
and find out what it does.
  87     g_return_val_if_fail(IS_BROWSERTOOLBAR(self), TRUE);
-atm the gnome and maemo-all indexes are unavailable, but this is basically an
evil yet uninteresting macro that would exit the function. but nothing
interesting has happened yet, and we're looking for something to happen, so
it's worth ignoring.
  88 #ifdef USE_HILDON
-browser wants to build on non hildon platforms.
  89 
  90     toolbar_set_wide_view(self);
-this sounds interesting
  91 #endif
  92 
  93     GTK_ENTRY(widget)->button = 0;
-this doesn't
  94 
  95     return FALSE;
-we ran out of lines.
  96 }

-click interesting
http://timeless.justdave.net/mxr-test/garage/ident?i=toolbar_set_wide_view
Defined as a function in:
    * browser/browser-ui/trunk/tablet-browser-ui/src/browsertoolbar_cb.c
          o line 28 
-we want this

Defined as a function prototype in:
    * browser/browser-ui/trunk/tablet-browser-ui/include/browsertoolbar_cb.h
          o line 68 
-hint...
    * browser/browser-ui/trunk/tablet-browser-ui/src/browsertoolbar_cb.c
          o line 90 
-we were here

Referenced in:
    * browser/browser-ui/trunk/tablet-browser-ui/src/browsertoolbar.c
          o line 197 
-we've been here, oh well. convenient that you can get here from more than one
place

-click what we want...
http://timeless.justdave.net/mxr-test/garage/source/browser/browser-ui/trunk/tablet-browser-ui/src/browsertoolbar_cb.c#28
  28 void toolbar_set_wide_view (BrowserToolbar *self)
-eliding because the top of the function is long and uninteresting.
  61     gtk_widget_grab_focus(GTK_WIDGET(self->address_entry));
-grab focus, expected, uninteresting
  62     gtk_editable_select_region(GTK_EDITABLE(self->address_entry), 0, -1);
-this sounds like it's doing precisely what the bug report describes. so
clearly the reporter doesn't like this line.
  63 
  64 #ifdef USE_HILDON
  65 
  66     gtk_im_context_show(GTK_ENTRY(self->address_entry)->im_context);
-this sounds like it'd show the input method context.

Ok, what's left? well, as it happens trunk urls are subject to change, we can
either try to get an svn url that won't or a branch url that won't.

To find other branches, we'd need to leave the nice, small, comfortable
/garage/, scroll to the top, view using tree: [garage-all|v].
Nothing should change, but we'll now have access to branches.

you'd navigate up to:
http://timeless.justdave.net/mxr-test/garage-all/source/browser/browser-ui/
and then pick branches and a branch, and then try to get back to about the same
file.
you'd end up here:
http://timeless.justdave.net/mxr-test/garage-all/source/browser/browser-ui/branches/trunk_200715_branch/maemo-browser-ui/src/browsertoolbar_cb.c#64

The alternative is also at the top of the page, on the right, click:
VC Blame
https://garage.maemo.org/plugins/scmsvn/viewcvs.php/browser-ui/trunk/tablet-browser-ui/src/browsertoolbar_cb.c?root=browser&view=annotate

Revision 125 - (view) (download) (as text)

Since this is svn, we can say that in revision 125,
62 :                 
gtk_editable_select_region(GTK_EDITABLE(self->address_entry), 0, -1); 
offends us.

Now, unfortunately, this may or may not be specified by the user interface
designer. (Odds are that it is specified.)

Which means that in order to fix it, we need to reverse engineer not just the
code (done) but the specification.

Why would the code be written this way? probably so that you can easily tap the
urlbar and replace the entire url.

How can we make this better? distinguish between tap and drag.

Proposal:
Only select the whole urlbar if the user taps but doesn't make a selection.

Exercise for the reader: figure out how to determine whether the user made a
selection.

This is where it'd help to have a working gnome or hildon cross reference. Or
at least sardine. Unfortunately the references I have at work include closed
pieces and given that the public server is already complaining about load and
my network is refusing to allow useful outbound transactions, I'm not in a
position to put up a replacement sardine.

Normally, i'd use the sardine reference to find out about:
 gtk_editable_select_region
especially the header declaration for it (yes, we kept skipping the headers,
but this is when we'd want to use them).

Instead, today, I used google.
https://www.linux-foundation.org/dbadmin/browse/int_single.php?Iname=gtk_editable_select_region
http://www.gtk.org/api/2.6/gtk/GtkEditable.html#id3019072

The next documented function is:
gtk_editable_get_selection_bounds ()

gboolean    gtk_editable_get_selection_bounds
                                            (GtkEditable *editable,
                                             gint *start,
                                             gint *end);

Gets the current selection bounds, if there is a selection.
editable :    a GtkEditable widget.
start :    location to store the starting position, or NULL.
end :    location to store the end position, or NULL.
Returns :    TRUE if there is a selection. 

Seems fairly simple.

Note: you should attach a patch at this point.
Comment 5 timeless 2007-08-30 09:02:11 UTC
Created an attachment (id=532) [details]
only select the whole address field if the user hasn't made a selection
Comment 6 timeless 2007-08-31 12:58:54 UTC
this is fixed in an internal svn. Unfortunately I can't give a commit url
because while there conveniently was a copy of the ui code open, it's actually
not the one that's being used (although it's clearly virtually identical as the
patch applied to the file perfectly and really did fix it).