[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kfm-devel
Subject:    Re: [Patch] Fixing the Access Key system
From:       bj () altern ! org
Date:       2004-07-14 10:39:02
Message-ID: 200407141246.58233.bj () altern ! org
[Download RAW message or body]

On Wednesday 14 July 2004 09.08, Lubos Lunak wrote:
>  I used Ctrl+Alt, because that's how Mozilla does it, and some pages
> advertize it that way (e.g. linuxtoday.com). The conflicts with global
> shortcuts are bad, but I'm afraid we can't require everybody to have
> 4-modifiers keyboards :(.
No, Mozilla and Firefox use the Alt+key system 
(http://www.mozilla.org/projects/ui/accessibility/accesskey.html)
I read many user reports that thought accesskeys didn't exist in Konqueror, 
and I only discovered the shortcut by reading the source code.

>  It'd be probably better if there was a normal shortcut for this rather
> than a single modifier key. For people preferring keyboard (and those are
> more likely to use accesskeys) Ctrl is not that rarely used key, and this
> could interfere. I've already noticed a similar problem with the
> Ctrl-suspends-Alt-arrows-scrolling feature, just e.g. switch virtual
> desktops (or even just intend to and then change your mind) and you have
> Ctrl pressed.

>  With your implementation global shortcuts actually don't work at all,
> because it grabs the keyboard, but that's hardly better (type-ahead find
> has the same problem). Can't you do without the grabs? If there's something
> possibly eating the keypresses before, simply temporarily install a global
> event filter and remove it again when done or on focus-out.
Well in fact I did put the grabkeyboard() because I took example on the 
type-ahead feature, but in fact it works well if I remove the 
grab/releasekeyboard() calls.
As my access key system proposal reacts on key release event, it can easily be 
adapted in such a way that it _only_ reacts if Ctrl was pressed and released 
and no other key was pressed in the meantime. (see new patch attached)
Also, I added a check, so that if the accesskey system is activated and you 
press any modifyer (ctrl, alt,...) it immediatly disables the accesskey 
sytem. 
With these extra checks, I don't think it can interfere anymore with other 
keyboards events.


> > Additionnaly, (and this can be removed from the patch if necessary), If
> > you press "Ctrl" again, small passive popups will appear in front of each
> > element in the page with an accesskey, showing that accesskey. 

>  Great idea. But I don't like the implementation - the passive popups are
> not really part of the html widget. Which means they don't scroll together
> with the page, and they stay on the screen if you change active
> window/desktop. They also don't go away after you actually press the
> accesskey. Would it be possible to e.g. use QLabel, reparent it to the
> khtml widget and keep them as the topmost children?
I agree, it was actually more a proof of concept. I first tried to do it with 
QTooltips, but couldn't find a way of making it work. Perhaps the QLabel way 
could be a better way...

>
> > This new behaviour could be advertised in konqueror's about: page.
> >
> > It would fix #45788 and #83053
>
>  #45788 probably can't be closed yet, as the accesskeys support is not
> complete. E.g. checkboxes still don't really work.

They should now work correctly since we recently committed a patch fixing the 
issue.

Attached is a new patch fixing a few conflicting issues and removing the 
display all access keys feature, since it probably needs a rewrite. At least, 
I think we should today commit a patch that displays a message in the status 
bar when the access key modifier is pressed. Will be back in 5 hours, 
comments welcome

regards
Jean-Baptiste

["accesskey_control.diff" (text/x-diff)]

Index: khtmlview.cpp
===================================================================
RCS file: /home/kde/kdelibs/khtml/khtmlview.cpp,v
retrieving revision 1.645
diff -u -3 -r1.645 khtmlview.cpp
--- khtmlview.cpp	13 Jul 2004 14:46:49 -0000	1.645
+++ khtmlview.cpp	14 Jul 2004 10:34:10 -0000
@@ -216,6 +218,8 @@
 #ifndef KHTML_NO_TYPE_AHEAD_FIND
         typeAheadActivated = false;
 #endif // KHTML_NO_TYPE_AHEAD_FIND
+	accessKeysActivated = false;
+	accessKeysPreActivate = false;
     }
     void newScrollTimer(QWidget *view, int tid)
     {
@@ -328,6 +332,9 @@
     bool findLinksOnly;
     bool typeAheadActivated;
 #endif // KHTML_NO_TYPE_AHEAD_FIND
+    bool accessKeysActivated;
+    bool accessKeysPreActivate;
+    QTimer accessKeysTimer;
 };
 
 #ifndef QT_NO_TOOLTIP
@@ -381,6 +388,7 @@
 #ifndef KHTML_NO_TYPE_AHEAD_FIND
     connect(&d->timer, SIGNAL(timeout()), this, SLOT(findTimeout()));
 #endif // KHTML_NO_TYPE_AHEAD_FIND
+    connect(&d->accessKeysTimer, SIGNAL(timeout()), this, \
SLOT(accessKeysTimeout()));  
     init();
 
@@ -1055,6 +1063,10 @@
     }
 #endif // KHTML_NO_CARET
 
+    // If CTRL was hit, be prepared for access keys
+    if (_ke->key() == Key_Control)
+	    d->accessKeysPreActivate=true;
+
     // accesskey handling needs to be done before dispatching, otherwise e.g. \
lineedits  // may eat the event
     if( handleAccessKey( _ke )) {
@@ -1137,7 +1149,7 @@
 		return;
 	}
 #endif // KHTML_NO_TYPE_AHEAD_FIND
-
+	
     int offs = (clipper()->height() < 30) ? clipper()->height() : 30;
     if (_ke->state() & Qt::ShiftButton)
       switch(_ke->key())
@@ -1349,6 +1361,16 @@
         _ke->accept();
         return;
     }
+    if (d->accessKeysPreActivate && _ke->key() != Key_Control) \
d->accessKeysPreActivate=false; +    if (d->accessKeysActivated && (_ke->state() != \
Qt::NoButton || _ke->state() != Qt::ShiftButton)) accessKeysTimeout(); +    if \
(_ke->key() == Key_Control &&  d->accessKeysPreActivate) +	{
+	    m_part->setStatusBarText(i18n("Access Keys activated")+i18n(" -- press Ctrl \
again to display all available access keys"),KHTMLPart::BarDefaultText); +	    \
d->accessKeysActivated = true; +	    d->accessKeysPreActivate = false;
+	    d->accessKeysTimer.start(5000, true);
+	}
+	
     QScrollView::keyReleaseEvent(_ke);
 }
 
@@ -1442,6 +1464,7 @@
 bool KHTMLView::eventFilter(QObject *o, QEvent *e)
 {
     if ( e->type() == QEvent::AccelOverride ) {
+    d->accessKeysPreActivate=false;
 	QKeyEvent* ke = (QKeyEvent*) e;
 //kdDebug(6200) << "QEvent::AccelOverride" << endl;
 	if (m_part->isEditable() || m_part->isCaretMode()
@@ -1802,12 +1825,19 @@
     }
 }
 
+
+void KHTMLView::accessKeysTimeout()
+{
+d->accessKeysActivated=false;
+d->accessKeysPreActivate = false;
+m_part->setStatusBarText(QString::null, KHTMLPart::BarDefaultText);
+}
+
 // Handling of the HTML accesskey attribute.
 bool KHTMLView::handleAccessKey( const QKeyEvent* ev )
 {
-    const int mods = Qt::AltButton | Qt::ControlButton;
-    if( ( ev->state() & mods ) != mods )
-        return false;
+if (!d->accessKeysActivated) return false;
+
 // Qt interprets the keyevent also with the modifiers, and ev->text() matches that,
 // but this code must act as if the modifiers weren't pressed
     QChar c;
@@ -1823,6 +1853,8 @@
     }
     if( c.isNull())
         return false;
+    d->accessKeysActivated=false;
+    accessKeysTimeout();
     return focusNodeWithAccessKey( c );
 }
 
Index: khtmlview.h
===================================================================
RCS file: /home/kde/kdelibs/khtml/khtmlview.h,v
retrieving revision 1.209
diff -u -3 -r1.209 khtmlview.h
--- khtmlview.h	30 Jun 2004 16:20:34 -0000	1.209
+++ khtmlview.h	14 Jul 2004 10:34:11 -0000
@@ -194,7 +194,6 @@
     void keyReleaseEvent ( QKeyEvent *_ke );
     void contentsContextMenuEvent ( QContextMenuEvent *_ce );
     void doAutoScroll();
-
     void timerEvent ( QTimerEvent * );
 
 protected slots:
@@ -206,7 +205,8 @@
 #ifndef KHTML_NO_TYPE_AHEAD_FIND
     void findTimeout();
 #endif // KHTML_NO_TYPE_AHEAD_FIND
-
+    void accessKeysTimeout();
+    
 private:
 
     void scheduleRelayout(khtml::RenderObject* clippedObj=0);



[prev in list] [next in list] [prev in thread] [next in thread]