Merge ~xnox/software-properties:clickable-pro into software-properties:ubuntu/master

Proposed by Dimitri John Ledkov
Status: Needs review
Proposed branch: ~xnox/software-properties:clickable-pro
Merge into: software-properties:ubuntu/master
Diff against target: 48 lines (+10/-2)
2 files modified
data/gtkbuilder/dialog-ua-attach.ui (+1/-1)
softwareproperties/gtk/DialogUaAttach.py (+9/-1)
Reviewer Review Type Date Requested Status
Nathan Teodosio (community) Disapprove
Ubuntu Core Development Team Pending
Review via email: mp+443188@code.launchpad.net

Commit message

dialog-ua-attach: attempt to make /pro/attach URL clickable

Just setting markup on the child label of the GtkRadioButton is not enough, as the URI does not get hover effect like the other urls (mouse pointer changing into a finger) and the URI itself is not clickable GTK style.

Ideally, I want clicking radio button to change radio button state, clicking "Enter code on" to do nothing, and only ubuntu.com/pro/attach URI to be clickable.

This is so far best I could do, but it would be nice if we could wire up a normal activate-link signals through to gtkradiobutton.

Note splitting gtkradiobutton into like hbox of empty radiobutton + gtklabel might work, but likely break a11y / screen readers.

To post a comment you must log in.
Revision history for this message
Nathan Teodosio (nteodosio) wrote :

The root of the problem might be motivated by the whole width of the screen being allocated as selection area for the button. So if a URL is present, the question is: Does clicking the URL launch the URL or activate the radio button, or both?

Your workaround answers "both" to that question, which I guess is fine — I wonder how designers would answer —, but introduces a side effect of having a browser window or tab open for each time the radio button is selected, also when keyboard navigation is used; I think this is too high a price to pay. One could use Pango Layout's get_pixel_size to get the label width, get the x position of the mouse click and then decide if the click was on the link or not, but it seems too much work and maybe it cannot be made accurate (because maybe there is no way to get the x coordinates interval the link occupies), so I would rather do the hbox and possibly confuse screen readers.

Now, the hbox workaround would introduce another effect: the button selection area would no longer be {window width, radio button icon height} but {radio button icon width, radio button icon height}. So for consistency sake we would do the same for the other radio button too.

What do you think?

Revision history for this message
Nathan Teodosio (nteodosio) wrote :

Exploring more of the *.ui options, it turns out accessibility directives are available; that mitigates or eliminates the concerns about screen readers.

E.g. by adding this to the radio button

                    <child internal-child="accessible">
                      <object class="AtkObject" id="magic_radio-atkobject">
                        <property name="AtkObject::accessible-name" translatable="yes">123</property>
                        <property name="AtkObject::accessible-description" translatable="yes">456</property>
                      </object>
                    </child>

The screen reader will say "123 selected radio button 456" when the first button is selected.

As such, I believe that hbox + accessibility directives is indeed the best approach to solve the issue at hand.

review: Disapprove

Unmerged commits

db4a1ee... by Dimitri John Ledkov

dialog-ua-attach: attempt to make /pro/attach URL clickable

LP: #2017770

Signed-off-by: Dimitri John Ledkov <email address hidden>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/data/gtkbuilder/dialog-ua-attach.ui b/data/gtkbuilder/dialog-ua-attach.ui
2index 1445b82..b42e69a 100644
3--- a/data/gtkbuilder/dialog-ua-attach.ui
4+++ b/data/gtkbuilder/dialog-ua-attach.ui
5@@ -32,7 +32,7 @@
6 <property name="orientation">vertical</property>
7 <child>
8 <object class="GtkRadioButton" id="magic_radio">
9- <property name="label" translatable="yes">Enter code on ubuntu.com/pro/attach</property>
10+ <property name="label" translatable="yes">Enter code on &lt;a href="https://ubuntu.com/pro/attach"&gt;ubuntu.com/pro/attach&lt;/a&gt;</property>
11 <property name="visible">True</property>
12 <property name="can-focus">True</property>
13 <property name="receives-default">False</property>
14diff --git a/softwareproperties/gtk/DialogUaAttach.py b/softwareproperties/gtk/DialogUaAttach.py
15index d029e64..18a0a2f 100644
16--- a/softwareproperties/gtk/DialogUaAttach.py
17+++ b/softwareproperties/gtk/DialogUaAttach.py
18@@ -20,7 +20,7 @@ import os
19 from gettext import gettext as _
20 import gi
21 gi.require_version("Gtk", "3.0")
22-from gi.repository import Gtk,GLib,Gio
23+from gi.repository import Gtk,GLib,Gio,Gdk
24 from softwareproperties.gtk.utils import setup_ui
25 from uaclient.api.u.pro.attach.magic.initiate.v1 import initiate
26 from uaclient.api.u.pro.attach.magic.wait.v1 import MagicAttachWaitOptions, wait
27@@ -31,6 +31,11 @@ class DialogUaAttach:
28 """setup up the gtk dialog"""
29 setup_ui(self, os.path.join(datadir, "gtkbuilder", "dialog-ua-attach.ui"), domain="software-properties")
30
31+ # GtkRadioButton doesn't have use_markup XML property
32+ # And this doesn't make the uri hover & be clickble like regular GtkLabels
33+ for child in self.magic_radio:
34+ child.set_use_markup(True)
35+
36 self.ua_object = ua_object
37 self.dialog = self.dialog_ua_attach
38 self.dialog.set_transient_for(parent)
39@@ -184,6 +189,9 @@ class DialogUaAttach:
40
41 def on_magic_radio_clicked(self, button):
42 self.start_magic_attach()
43+ # Even with set_use_markup on the child of GtkRadioButton, doesn't create hover, tooltip, or activate-link events, hence this manual url opener
44+ if self.magic_radio.get_active():
45+ Gtk.show_uri_on_window(None, "https://ubuntu.com/pro/attach", Gdk.CURRENT_TIME)
46
47 # Do not control the radio buttons and confirm button widgets directly,
48 # since those former are controlled by update_state and this function must

Subscribers

People subscribed via source and target branches