Bug 32509 - Composition input method lacks character echo in password input fields
Summary: Composition input method lacks character echo in password input fields
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Hardware S60 3rd edition
: P2 Major
Assignee: Chang Shu
URL:
Keywords:
Depends on: 59114 66052
Blocks: 27065 66561
  Show dependency treegraph
 
Reported: 2009-12-14 04:56 PST by Petri Ojala
Modified: 2011-08-25 12:24 PDT (History)
32 users (show)

See Also:


Attachments
testpage (54 bytes, text/html)
2009-12-14 05:17 PST, Petri Ojala
no flags Details
proposed patch (6.74 KB, patch)
2010-01-08 02:47 PST, Petri Ojala
no flags Details | Formatted Diff | Diff
Updated patch (6.74 KB, patch)
2010-01-08 03:01 PST, Petri Ojala
hausmann: review-
Details | Formatted Diff | Diff
patch (12.43 KB, patch)
2010-04-19 04:41 PDT, Samuel Nevala
no flags Details | Formatted Diff | Diff
patch (12.43 KB, patch)
2010-04-19 04:48 PDT, Samuel Nevala
no flags Details | Formatted Diff | Diff
patch (12.08 KB, patch)
2010-04-19 04:59 PDT, Samuel Nevala
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
Latest_Patch (22.82 KB, patch)
2010-05-17 04:59 PDT, Samuel Nevala
no flags Details | Formatted Diff | Diff
La (22.82 KB, patch)
2010-05-17 21:52 PDT, Samuel Nevala
no flags Details | Formatted Diff | Diff
Latest_Patch (22.82 KB, patch)
2010-05-17 21:53 PDT, Samuel Nevala
no flags Details | Formatted Diff | Diff
Latest patch (13.33 KB, patch)
2010-06-18 02:10 PDT, Samuel Nevala
rniwa: review-
Details | Formatted Diff | Diff
fix patch (13.83 KB, patch)
2011-01-17 14:44 PST, Chang Shu
no flags Details | Formatted Diff | Diff
fix patch 2 (15.25 KB, patch)
2011-01-19 12:09 PST, Chang Shu
no flags Details | Formatted Diff | Diff
fix patch 3 (15.48 KB, patch)
2011-01-28 13:58 PST, Chang Shu
eric: review-
Details | Formatted Diff | Diff
fix patch 3: rebaseline + minor changes (14.04 KB, patch)
2011-06-07 07:10 PDT, Chang Shu
no flags Details | Formatted Diff | Diff
patch 4 (15.20 KB, patch)
2011-07-06 08:59 PDT, Chang Shu
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.21 MB, application/zip)
2011-07-06 10:22 PDT, WebKit Review Bot
no flags Details
patch 5 (21.17 KB, patch)
2011-07-07 12:46 PDT, Chang Shu
no flags Details | Formatted Diff | Diff
patch 6 (20.28 KB, patch)
2011-07-11 12:20 PDT, Chang Shu
no flags Details | Formatted Diff | Diff
patch 7: follow up Antonio's comments (20.30 KB, patch)
2011-07-11 14:21 PDT, Chang Shu
cshu: review-
Details | Formatted Diff | Diff
patch 8: addresses ap's review, etc. (20.63 KB, patch)
2011-07-12 11:33 PDT, Chang Shu
ap: review-
Details | Formatted Diff | Diff
patch 9 (11.65 KB, patch)
2011-08-18 08:28 PDT, Chang Shu
no flags Details | Formatted Diff | Diff
patch 10 (9.08 KB, patch)
2011-08-18 12:53 PDT, Chang Shu
ap: review+
Details | Formatted Diff | Diff
patch 11: r+ed with minor changes (9.02 KB, patch)
2011-08-18 18:53 PDT, Chang Shu
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch 12: fix the crash on chromium (11.07 KB, patch)
2011-08-23 13:55 PDT, Chang Shu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Petri Ojala 2009-12-14 04:56:43 PST
This bug report originated from issue QTBUG-6704
http://bugreports.qt.nokia.com/browse/QTBUG-6704

For non-touch screen phones, the input character is shown as *, which is correct of course. But i have no idea what i have typed in, in small letter or capital letter. For the touch-sreen input method, it will show you the letter that you tpyed in and then displays it as *.

My suggestion is to improve the input method for non-touch screens, while using the keyboard to fill in the letters, the letter filled in should be displayed, when confirmed, it shows *, exactly as how it is working on the touch-screen.

So, it should have character echo before echoing *.
Comment 1 Petri Ojala 2009-12-14 05:17:59 PST
Created attachment 44787 [details]
testpage
Comment 2 Petri Ojala 2009-12-14 05:20:12 PST
Can be reproduced with N97 in landscape mode
Comment 3 Janne Koskinen 2009-12-14 05:27:56 PST
This is serious usability issue when using T9 as if you can't see the character being echoed you need to count the presses to know which character you entered.
Comment 4 Simon Hausmann 2009-12-14 08:28:29 PST
I wonder if this could be solved inside qcoefepinputcontext_s60.cpp, so that it would also work with QLineEdit's password mode and the code wouldn't have to be duplicated.

 Do you think that's possible/feasible/makes sense?
Comment 5 Janne Koskinen 2009-12-15 04:15:02 PST
(In reply to comment #4)
> I wonder if this could be solved inside qcoefepinputcontext_s60.cpp, so that it
> would also work with QLineEdit's password mode and the code wouldn't have to be
> duplicated.
> 
>  Do you think that's possible/feasible/makes sense?

A common place for echoing would be good idea indeed. I tried simple QLineEdit and the feature is completely missing. I'll change the Qt bug to better reflect the situation.
Comment 6 Simon Hausmann 2009-12-15 05:18:46 PST
(In reply to comment #5)
> (In reply to comment #4)
> > I wonder if this could be solved inside qcoefepinputcontext_s60.cpp, so that it
> > would also work with QLineEdit's password mode and the code wouldn't have to be
> > duplicated.
> > 
> >  Do you think that's possible/feasible/makes sense?
> 
> A common place for echoing would be good idea indeed. I tried simple QLineEdit
> and the feature is completely missing. I'll change the Qt bug to better reflect
> the situation.

Hmm, I actually can't think of a way to achieve this from just the input method, as it merely affects the rendering of the character.

That means it is a feature of the rendering container (WebKit and QLineEdit) and not a feature of the input method.
Comment 7 Joseph Ligman 2009-12-15 06:57:20 PST
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > I wonder if this could be solved inside qcoefepinputcontext_s60.cpp, so that it
> > > would also work with QLineEdit's password mode and the code wouldn't have to be
> > > duplicated.
> > > 
> > >  Do you think that's possible/feasible/makes sense?
> > 
> > A common place for echoing would be good idea indeed. I tried simple QLineEdit
> > and the feature is completely missing. I'll change the Qt bug to better reflect
> > the situation.
> 
> Hmm, I actually can't think of a way to achieve this from just the input
> method, as it merely affects the rendering of the character.
> 
> That means it is a feature of the rendering container (WebKit and QLineEdit)
> and not a feature of the input method.


What we did for S60 Browser in the past: (with a few details omitted)

In RenderText.cpp 


#if PLATFORM(SYMBIAN)

void RenderText::securityTimerFired(Timer<RenderText>*)
{
    switch (style()->textSecurity()) {
        case TSCIRCLE:
            m_text = m_text->secure(whiteBullet);
            break;
        case TSDISC:
            m_text = m_text->secure(bullet);
            break;
        case TSSQUARE:
            m_text = m_text->secure(blackSquare);
    }
    setNeedsLayoutAndPrefWidthsRecalc();
}

#endif

#if PLATFORM(SYMBIAN)
	
        // We use the same characters here as for list markers.
        // See the listMarkerText function in RenderListMarker.cpp.
        switch (style()->textSecurity()) {
            case TSNONE:
                break;
            case TSCIRCLE:
                m_text = m_text->secureShowOffset(bullet, m_offset);
                break;
            case TSDISC:
                if(backspace){
                        m_text = m_text->secure(bullet);
                }
                else{
                    m_text = m_text->secureShowOffset(bullet, m_offset);
                }
                       
               break;
            case TSSQUARE:
                m_text = m_text->secureShowOffset(blackSquare, m_offset);
        }
       
        if (style()->textSecurity() != TSNONE) {
            m_securityTimer.stop();
            m_securityTimer.startOneShot(KSecureTimerTimeout);
        }
       
#else 
...
Comment 8 Petri Ojala 2010-01-08 02:47:44 PST
Created attachment 46124 [details]
proposed patch

Attaching patch.
Comment 9 WebKit Review Bot 2010-01-08 02:53:30 PST
Attachment 46124 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderText.h:183:  Should have a space between // and comment  [whitespace/comments] [4]
WebCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:18:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4
Comment 10 Petri Ojala 2010-01-08 03:01:29 PST
Created attachment 46125 [details]
Updated patch

Updated patch
Comment 11 WebKit Review Bot 2010-01-08 03:04:15 PST
style-queue ran check-webkit-style on attachment 46125 [details] without any errors.
Comment 12 Kristian Amlie 2010-01-15 08:18:54 PST
I think a much more correct fix for this problem is to implement the timer logic in the input methods of Qt, by having it sending preedit text to the widget and only commit it after the timer expires. That would be a generic fix that would work for all Qt widgets and webkit as well. It would render this bug moot, so I'm going to try that next week.
Comment 13 Simon Hausmann 2010-01-16 00:42:57 PST
Comment on attachment 46125 [details]
Updated patch

At the least this should not be just be PLATFORM(SYMBIAN) #ifdef'fed. The same feature is needed for example on Maemo 5, too.

However if Kristian can solve this inside Qt's input method, then that's of course the cleanest solution, as it would fix all occurrences of this problem in one place.
Comment 14 Kristian Amlie 2010-01-25 02:51:33 PST
Ok, this has been solved for general Qt widgets by introducing a temporary 1-second preedit text in the input methods that Qt uses (commit 87ee066fc86 in the Qt repository).

This bug is still valid though, since WebKit does not display preedit text as clear text, but masks that also as password text. This is IMO a bug, since this makes any kind of advanced input into a password field nearly impossible. The suggested fix is now to fix WebKit to display this preedit text also in password fields. Whether or not you want to reuse this bug report for that issue or not, I leave to you.
Comment 15 Samuel Nevala 2010-04-19 04:41:59 PDT
Created attachment 53669 [details]
patch
Comment 16 WebKit Review Bot 2010-04-19 04:44:49 PDT
Attachment 53669 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/editing/ReplaceTextInNodeCommand.h:20:  #ifndef header guard has wrong style, please use: ReplaceTextInNodeCommand_h  [build/header_guard] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Samuel Nevala 2010-04-19 04:48:01 PDT
Created attachment 53670 [details]
patch
Comment 18 WebKit Review Bot 2010-04-19 04:51:33 PDT
Attachment 53670 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/editing/ReplaceTextInNodeCommand.h:20:  #ifndef header guard has wrong style, please use: ReplaceTextInNodeCommand_h  [build/header_guard] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Samuel Nevala 2010-04-19 04:59:20 PDT
Created attachment 53671 [details]
patch
Comment 20 WebKit Review Bot 2010-04-19 05:07:39 PDT
Attachment 53669 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1602587
Comment 21 WebKit Review Bot 2010-04-19 05:26:01 PDT
Attachment 53671 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1569608
Comment 22 Antonio Gomes 2010-04-19 06:14:52 PDT
(In reply to comment #19)
> Created an attachment (id=53671) [details]
> patch

I think the approach looks good, but unless you improve the test covarage of this feature/fix, and make it build all perrs plaftorms/ports (including Mac and chromium), it can be harder to be accepted, so I'd work on LayoutTests for this.

Also Mac would likely fail to build your patch as well.
Comment 23 Simon Hausmann 2010-04-19 16:41:24 PDT
Comment on attachment 53671 [details]
patch

I can do an initial review on the generics, but for exact workings of the editing code
I'm not the right one to do the review.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index c27fce0..c3c01e1 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,25 @@
> +2010-04-19  Samuel Nevala  <samuel.nevala@digia.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt] 3rd edition phones lack character echo in web input fields
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=32509
> +

Please explain your change in the changelog.

> --- a/WebCore/WebCore.pro
> +++ b/WebCore/WebCore.pro
> @@ -568,6 +568,7 @@ SOURCES += \
>      editing/RemoveNodePreservingChildrenCommand.cpp \
>      editing/ReplaceNodeWithSpanCommand.cpp \
>      editing/ReplaceSelectionCommand.cpp \
> +    editing/ReplaceTextInNodeCommand.cpp \
>      editing/SelectionController.cpp \
>      editing/SetNodeAttributeCommand.cpp \
>      editing/SmartReplace.cpp \
> @@ -1289,6 +1290,7 @@ HEADERS += \
>      editing/RemoveNodePreservingChildrenCommand.h \
>      editing/ReplaceNodeWithSpanCommand.h \
>      editing/ReplaceSelectionCommand.h \
> +    editing/ReplaceTextInNodeCommand.h \
>      editing/SelectionController.h \
>      editing/SetNodeAttributeCommand.h \
>      editing/SmartReplace.h \

When adding new files to the build I think you'll have to add them to the
other build systems, too.

> +UChar CharacterData::secureTextChar()

Shouldn't this function be const?
Comment 24 Samuel Nevala 2010-04-20 02:43:49 PDT
>>I think the approach looks good, but unless you improve the test covarage of
>>this feature/fix, and make it build all perrs plaftorms/ports (including Mac
>>and chromium), it can be harder to be accepted, so I'd work on LayoutTests for
>>this.
>>Also Mac would likely fail to build your patch as well.

I don't have mac in use. It is bit difficult to verify mac build and I would appreciate help on that side.

About layout tests, is it possible to acquire secured pass text using test environment? By secured pass text I mean text value converted to secure chars.

>>When adding new files to the build I think you'll have to add them to the
>>other build systems, too.

I assuming that you mean adding new files also to Android.mk & GNUmakefile.am. I there other places where new file should be added?

>>Shouldn't this function be const?
Yes, I'll correct that.
Comment 25 Samuel Nevala 2010-05-17 04:59:07 PDT
Created attachment 56235 [details]
Latest_Patch
Comment 26 WebKit Review Bot 2010-05-17 05:01:12 PDT
Attachment 56235 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "WebKit/qt/tests/qwebpage/tst_qwebpage.cpp"
WebCore/dom/CharacterData.cpp:164:  Missing spaces around ==  [whitespace/operators] [3]
WebCore/dom/CharacterData.cpp:229:  Missing space before ( in if(  [whitespace/parens] [5]
WebCore/dom/CharacterData.cpp:229:  Mismatching spaces inside () in if  [whitespace/parens] [5]
WebCore/rendering/RenderText.cpp:1077:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 4 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 Samuel Nevala 2010-05-17 21:52:24 PDT
Created attachment 56319 [details]
La
Comment 28 Samuel Nevala 2010-05-17 21:53:37 PDT
Created attachment 56320 [details]
Latest_Patch

Fixed style.
Comment 29 WebKit Review Bot 2010-05-21 03:29:15 PDT
Attachment 56320 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2258404
Comment 30 Janne Koskinen 2010-06-10 00:11:56 PDT
buildbot has false positive on this?

Can someone review this as again without the patch in Symbian devices you cannot enter password protected websites.
Comment 31 Simon Hausmann 2010-06-10 04:50:05 PDT
I've briefly spoken with Samuel also on irc, but here's my take on this:

As far as I can see the information about whether the text should be shown plain or "secured" (the text security) is stored in the render style. The logic for displaying the text is in the render object. Therefore I think the logic for implementing this feature should also be in the render object (RenderText, RenderTextControlSingleLine, etc.).

I think adding this feature specific preedit flag to WebCore::String is not a good idea.
Comment 32 Simon Hausmann 2010-06-17 03:34:45 PDT
Comment on attachment 56320 [details]
Latest_Patch

AFAIK Samuel is working on a new revision of the same patch but in a much less intrusive way. Therefore I'm clearing the review for this patch.
Comment 33 Samuel Nevala 2010-06-18 02:10:23 PDT
Created attachment 59086 [details]
Latest patch

Layout test run on Linux. Qt auto test run on 5.0 sdk emulator. Fix also testes on Nokia XpressMusic as described on bug report. No regression detected.

Linux:
- qt 4.6.2
- git://gitorious.org/webkit/webkit.git head=ef989587e6fc7dba53992b0fc2f45703333061a7

s60 emulator & hw:
- git://gitorious.org/+qt-s60-developers/qt/qt-s60.git head=442784de39c8128f418354fdcfdb3988bb599104
- git://gitorious.org/webkit/webkit.git head=9d9ada9106cb31e6ea00e7eacb9c49aa90652769
Comment 34 Simon Hausmann 2010-06-18 08:16:15 PDT
Antti, could you help reviewing this patch?

The basic context is that for password fields on devices with virtual keyboards we want to display the last entered character temporarily before it turns into a * as password character.

Qt's input method - for password fields - will submit the last entered character as part of the composition and a second later it'll submit it as committed string. With this patch Samuel is trying to make sure that the password character that is part of the composition is shown in clear text.

I like this patch for being small and less intrusive, but I see that it is technically a layer violation. Do you think the violation is acceptible or is there maybe a better way of doing it?


Thanks :)
Comment 35 Kenneth Rohde Christiansen 2010-06-21 13:52:52 PDT
(In reply to comment #34)
> Antti, could you help reviewing this patch?
> 
> The basic context is that for password fields on devices with virtual keyboards we want to display the last entered character temporarily before it turns into a * as password character.
> 
> Qt's input method - for password fields - will submit the last entered character as part of the composition and a second later it'll submit it as committed string. With this patch Samuel is trying to make sure that the password character that is part of the composition is shown in clear text.
> 
> I like this patch for being small and less intrusive, but I see that it is technically a layer violation. Do you think the violation is acceptible or is there maybe a better way of doing it?
> 
> 
> Thanks :)

We should really avoid adding mor layer violations. Anyway, this seems to be the same behaviour on Maemo/MeeGo, so I guess this bug is not really Symbian specific.
Comment 36 Antti Koivisto 2010-06-24 03:34:24 PDT
I don't understand the symbian editing code (or editing in general) well enough to really give useful comments. However I find it surprising that the changelog talks about symbian only yet all the code changes are in the generic code? How is this going to affect other platforms? Is this what you mean by layer violation here?
Comment 37 Simon Hausmann 2010-06-24 07:57:04 PDT
(In reply to comment #36)
> I don't understand the symbian editing code (or editing in general) well enough to really give useful comments. However I find it surprising that the changelog talks about symbian only yet all the code changes are in the generic code? How is this going to affect other platforms? Is this what you mean by layer violation here?

The bug itself is not Symbian specific, even though it was ported as such. Other platforms that implement virtual keyboards through input methods suffer from the same problem I believe.
Comment 38 Samuel Nevala 2010-07-02 02:56:52 PDT
if (!document()->page()->focusController()->focusedOrMainFrame()->editor()->hasComposition())    m_text.makeSecure(secureChar);

can be replaced with 

if(!(node() && document()->frame()->editor()->compositionNode() == node()))                m_text.makeSecure(secureChar);

and #include "FocusController.h" can be removed. Layer violation is there already if it is about document()->frame()->editor(). To me it seem pretty minor.
Comment 39 Chang Shu 2010-07-15 07:19:07 PDT
This "character echo" feature should only be valid for mobile devices and ITUT keyboard. For other cases that user knows exactly what he types, we should not expose the character due to security reasons. For example, on desktop, you don't want to echo password as some other guys may look over your shoulder. :)
So I think we should either have the code in platform-specific places or make it configurable and enable it only on certain scenarios.
Comment 40 Kristian Amlie 2010-07-15 07:40:22 PDT
(In reply to comment #39)
> This "character echo" feature should only be valid for mobile devices and ITUT keyboard. For other cases that user knows exactly what he types, we should not expose the character due to security reasons. For example, on desktop, you don't want to echo password as some other guys may look over your shoulder. :)
> So I think we should either have the code in platform-specific places or make it configurable and enable it only on certain scenarios.

Why? I think it makes sense even on desktop to allow input method composed characters to be visible. I will admit I am not a user of any advanced input methods (like Chinese or similar), so I cannot speak on their behalf obviously, but I have the impression that fairly advanced compositions more or less require you to be able to see what you type.

And of course, typing in English or any other regular alphabets would not show the text.
Comment 41 Chang Shu 2010-07-15 08:04:31 PDT
(In reply to comment #40)
> (In reply to comment #39)
> > This "character echo" feature should only be valid for mobile devices and ITUT keyboard. For other cases that user knows exactly what he types, we should not expose the character due to security reasons. For example, on desktop, you don't want to echo password as some other guys may look over your shoulder. :)
> > So I think we should either have the code in platform-specific places or make it configurable and enable it only on certain scenarios.
> 
> Why? I think it makes sense even on desktop to allow input method composed characters to be visible. I will admit I am not a user of any advanced input methods (like Chinese or similar), so I cannot speak on their behalf obviously, but I have the impression that fairly advanced compositions more or less require you to be able to see what you type.
> 
> And of course, typing in English or any other regular alphabets would not show the text.

I see your point of showing composed characters even on desktop. It's a trade off but to me, exposing password in public is much more serious than lacking character echo. I am not sure if we have a real case here, either. For example, in Chinese, passwords are still in English alphabets.

In addition, is it so that the patch submitted only deals with composed characters?
Comment 42 Kristian Amlie 2010-07-16 00:30:30 PDT
(In reply to comment #41)
> I see your point of showing composed characters even on desktop. It's a trade off but to me, exposing password in public is much more serious than lacking character echo. I am not sure if we have a real case here, either. For example, in Chinese, passwords are still in English alphabets.

In that case they will probably never see the effects of this patch.

> In addition, is it so that the patch submitted only deals with composed characters?

Yes.
Comment 43 Janne Koskinen 2010-11-22 00:33:29 PST
Bump. You can reproduce this issue on N8 using portrait VKB.
Comment 44 Prasad 2011-01-06 14:01:41 PST
Related QtWRT Bugzilla error ID: http://bugs.nokia-boston.com/bugzilla/show_bug.cgi?id=7298
Comment 45 Antonio Gomes 2011-01-08 20:58:23 PST
I do not see how this bug is Qt-only (see [Qt] prefix in bug title). It only touches cross platform code.
Comment 46 Chang Shu 2011-01-09 08:51:21 PST
I believe it's just historical reason that we set the [qt] flag.
Comment 47 Ryosuke Niwa 2011-01-09 12:55:53 PST
Comment on attachment 59086 [details]
Latest patch

View in context: https://bugs.webkit.org/attachment.cgi?id=59086&action=review

> LayoutTests/editing/input/secure-text.html:56
> +        eventSender.keyDown("rightArrow");
> +    }

You should probably log("DONE") at the end.

> WebCore/editing/Editor.cpp:1487
> +           RenderText* textRenderer = static_cast<RenderText*>(baseNode->renderer());
> +           if (textRenderer && textRenderer->style() && textRenderer->style()->textSecurity() != TSNONE) {
> +               String originalText = textRenderer->text();
> +               originalText.replace(baseOffset, text.length(), text);
> +               textRenderer->setText(originalText.impl());
> +           }

Why aren't you fixing TypingCommand::insertText instead (e.g. add an argument)?  It doesn't make much sense to me that you're inserting text, and then replacing it.  This code may affect selection; e.g. clears selection.

> WebCore/rendering/RenderText.cpp:1098
> +            if (!document()->page()->focusController()->focusedOrMainFrame()->editor()->hasComposition())
> +                m_text.makeSecure(secureChar);

How do you know that this text node is the one focused?  Or even that m_text is on the focused frame? r- because of this condition.
Comment 48 Alexey Proskuryakov 2011-01-09 13:26:19 PST
+        [Qt] 3rd edition phones lack character echo in web input fields

Is this bug about password inputs, or more general?

Displaying the latest character typed into a password field sounds like something that already works on iOS. Did you check how that's implemented? Perhaps the code is already in open source WebCore, and no changes are necessary?

+    function seucreText(textLength)

Typo: seucre.

+            if (style()->textSecurity() == TSCIRCLE)
+                secureChar = whiteBullet;
+            else if (style()->textSecurity() == TSDISC)
+                secureChar = bullet;
+            else if (style()->textSecurity() == TSSQUARE)
+                secureChar = blackSquare;

Replacing a switch with an if/else sequence is not great, because that way, the compiler won't complain if more options are added to TextSecurity enum, and we forget to update this code.
Comment 49 Janne Koskinen 2011-01-10 01:08:47 PST
(In reply to comment #48)
> Is this bug about password inputs, or more general?

It is about password input echoing with multi-state-key keyboard. Issue exists also if language which combines characters to make syllables or words is being entered into password field.

> Displaying the latest character typed into a password field sounds like something that already works on iOS. Did you check how that's implemented? 

AFAIK iOS devices don't have ITU-T keyboard so unlikely this has become an issue there.

There is workarounds for this bug all over platforms, least the trackers that I can access.

Anyways, Samuel has not worked on webkit for 6 months and thus you most likely won't get reply from him unless he feels generous. So, if someone wants this to get fixed better pick it up and continue.
Comment 50 Alexey Proskuryakov 2011-01-10 01:26:06 PST
I don't even know what an ITU-T keyboard is, but iOS just always displays the last character in the password field for a second or two. This sounds like what this patch tries to make possible, too.
Comment 51 Kristian Amlie 2011-01-10 01:49:43 PST
> I don't even know what an ITU-T keyboard is, but iOS just always displays the last character in the password field for a second or two. This sounds like what this patch tries to make possible, too.

How does it display the last character? Is it just a property of the editor, or does the platform code have to send any specific events to show and hide the character?

From the Qt side it does not matter whether it's an ITU-T keyboard or not. All keyboards, when used in password fields, will send characters of the "preedit" type, which means they should be displayed, even in password fields. Then after a second, it erases the character, and sends a new "committed" character.
Comment 52 Janne Koskinen 2011-01-10 01:52:28 PST
(In reply to comment #50)
> I don't even know what an ITU-T keyboard is, but iOS just always displays the last character in the password field for a second or two. This sounds like what this patch tries to make possible, too.

Right, that is exactly what this patch is trying to make.

ITU-T E.161 http://en.wikipedia.org/wiki/E.161 ; commonly referred as ITU-T keyboard.

The original patch was based on S60 browser changes also posted on this bug report #c7 and from there it started to live as now Qt does the correct thing on passing input events to webkit.
Comment 53 Chang Shu 2011-01-10 06:04:42 PST
> Anyways, Samuel has not worked on webkit for 6 months and thus you most likely won't get reply from him unless he feels generous. So, if someone wants this to get fixed better pick it up and continue.

I talked to Samuel through email before and I will pick up the rest of the work unless he's available now.
Comment 54 David Kilzer (:ddkilzer) 2011-01-11 09:06:52 PST
(In reply to comment #51)
> > I don't even know what an ITU-T keyboard is, but iOS just always displays the last character in the password field for a second or two. This sounds like what this patch tries to make possible, too.
> 
> How does it display the last character? Is it just a property of the editor, or does the platform code have to send any specific events to show and hide the character?

The iOS port of WebKit uses a timer to determine when to obscure the last character in a text field.  The code is mostly self-contained within WebCore/rendering/RenderText.cpp (as I recall).
Comment 55 Chang Shu 2011-01-13 06:14:59 PST
> 
> The iOS port of WebKit uses a timer to determine when to obscure the last character in a text field.  The code is mostly self-contained within WebCore/rendering/RenderText.cpp (as I recall).

Right, I just checked the iOS code and want to confirm this. Function CharacterData::insertData() calls RenderText::momentarilyRevealLastCharacter(), which starts a timer to do the text replacement.

Personally, I prefer Samuel's approach. I will work on the next patch based on Niwa's review.
Comment 56 Chang Shu 2011-01-13 06:56:54 PST
I forgot to ask: does Samuel's patch work for all platforms? If it's a Qt-only solution, we'd better go for the iOS timer approach.
Comment 57 Kristian Amlie 2011-01-13 07:40:00 PST
It will probably not work for all platforms, because I don't expect that all of them use composition text in their password fields to temporarily display the character. Instead, I would assume they just send normal text, and it is up to RenderText.cpp to display the temporary character.

If you do use the iOS approach, keep in mind that the first version of the character that arrives from Qt will be composition (preedit) text, and after a second or so it is replaced by real text, so in that case you will have two timers running.
Comment 58 Prasad 2011-01-13 08:37:41 PST
Nokia Internal Bugzilla error ID: 
http://bugs.nokia-boston.com/bugzilla/show_bug.cgi?id=7298
Comment 59 Alexey Proskuryakov 2011-01-13 09:06:33 PST
> Personally, I prefer Samuel's approach.

Are you suggesting that we add new code to WebCore for another way to do something that's already supported by it?
Comment 60 Chang Shu 2011-01-13 10:14:26 PST
(In reply to comment #59)
> > Personally, I prefer Samuel's approach.
> 
> Are you suggesting that we add new code to WebCore for another way to do something that's already supported by it?

It seems Samuel's patch won't work across all platforms so I will proceed with iOS approach.
Comment 61 Kenneth Rohde Christiansen 2011-01-13 10:22:12 PST
Please do this in a way that it will work with Qt/WebKit2. Our development branch in gitorious has support for input methods.
Comment 62 Janne Koskinen 2011-01-14 05:25:16 PST
(In reply to comment #60)
> (In reply to comment #59)
> > > Personally, I prefer Samuel's approach.
> > 
> > Are you suggesting that we add new code to WebCore for another way to do something that's already supported by it?
> 
> It seems Samuel's patch won't work across all platforms so I will proceed with iOS approach.

If you look at the patches there is the iOS approach https://bugs.webkit.org/attachment.cgi?id=46125 .
Comment 63 Chang Shu 2011-01-14 08:28:09 PST
> 
> If you look at the patches there is the iOS approach https://bugs.webkit.org/attachment.cgi?id=46125 .

Thanks, Janne. I found the iOS source code from the web. The implementation is slightly different. I will provide a new patch soon.
Comment 64 Chang Shu 2011-01-14 11:41:12 PST
I am working on the test case right now. However, I couldn't figure out how composition mode was turned on in the test case in Samuel's patch. It looks to me the password echo is always on in iOS source code. It may not be an issue in iOS but we need a flag for webkit trunk. Using editor()->hasComposition() seems to be a good choice but I don't know how we can enable it through layout tests. I am not able to do it manually on any desktop browsers either because IME is not allowed for password field. The only place to make it work is a phone device with ITU-T keyboard. Another option is to use a flag in page/settings.h. Any inputs, everybody? Thanks!
Comment 65 Chang Shu 2011-01-17 14:44:43 PST
Created attachment 79219 [details]
fix patch
Comment 66 Antonio Gomes 2011-01-17 15:24:03 PST
Comment on attachment 79219 [details]
fix patch

This goes against what was discussed here: http://old.nabble.com/Bools-are-strictly-worse-than-enums-td30371586.html
Comment 67 Ryosuke Niwa 2011-01-17 15:39:56 PST
Comment on attachment 79219 [details]
fix patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79219&action=review

Although I'm not qualified to r+ a patch for this bug, I'll still give you my feedback.

> Source/WebCore/dom/CharacterData.h:37
> -    void insertData(unsigned offset, const String&, ExceptionCode&);
> +    void insertData(unsigned offset, const String&, ExceptionCode&, bool canShowLastCharacterIfSecure = false);

Nit: I'm not sure if canShowLastCharacterIfSecure is the right name. We certainly show last character when it's not secure.  Maybe canShowSecureLastCharacter?

> Source/WebCore/rendering/RenderText.cpp:1544
> +    Timer<RenderText>* secureLastCharacterTimer;
> +    if (m_hasSecureLastCharacterTimer) {
> +        secureLastCharacterTimer = gSecureLastCharacterTimers->get(this);
> +        secureLastCharacterTimer->stop();
> +    } else {
> +        secureLastCharacterTimer = new Timer<RenderText>(this, &RenderText::secureLastCharacter);
> +        gSecureLastCharacterTimers->add(this, secureLastCharacterTimer);
> +        m_hasSecureLastCharacterTimer = true;
> +    }

Mn... what happens if Timer fires after RenderText is destroyed?  Or is there an guarantee that it won't happen?

> LayoutTests/editing/input/secure-text.html:65
> +    if (testIdx >= 0) {
> +        expectedSecureTextLen = tests[testIdx][2];
> +        textInputController.doCommand("moveForward:");
> +        assert(tests[testIdx][4], window.find(secureText(expectedSecureTextLen), false, true), "secured after delay.");
> +    }

It'll be nice if you could put this verification in a closure or something so that the verification code appears after the test code.  You can have two functions test & verify and make them mutually "recursive" via setTimeouts.

> LayoutTests/editing/input/secure-text.html:78
> +    for(var i = 0; i < charSequence.length - 1; i++) {

Nit: space between for and (

> LayoutTests/editing/input/secure-text.html:87
> +    window.setTimeout(run, 1200);

1200 seems like an awfully long time.  Can we shorten it to something like 500?  Or will that break the test / make the test flaky?

> LayoutTests/editing/input/secure-text.html:96
> +    if (window.layoutTestController) {
> +        layoutTestController.dumpAsText();
> +        layoutTestController.waitUntilDone();
> +        run();
> +    }

Note that textInputController isn't implemented on all platforms so you might want to check window.textInputController and print out appropriate message here.
Comment 68 Kenneth Rohde Christiansen 2011-01-17 15:41:04 PST
Comment on attachment 79219 [details]
fix patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79219&action=review

> Source/WebCore/dom/CharacterData.h:37
> +    void insertData(unsigned offset, const String&, ExceptionCode&, bool canShowLastCharacterIfSecure = false);

Ah I see you use MomentarilyRevealLastCharacter above.

Maybe, enum SecureMode { AlwaysHideCharacters, MomentarilyRevealLastCharacter }

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:53
> +    m_node->insertData(m_offset, m_text, ec, canShowLastCharacterIfSecure);

Is it really "canShow...", it feels more like a "doShow". Maybe I would make make it clear that it is temporarily. ie showLastCharTemporarilyIfSecure.

> Source/WebCore/rendering/RenderText.h:122
> +    void secureLastCharacter(Timer<RenderText> * aTimer);

There should be no space before *, also you can remove aTimer.
Comment 69 Chang Shu 2011-01-18 07:19:57 PST
Thanks for the reviews and comments. Please see my comments below.

> Source/WebCore/dom/CharacterData.h:37
> +    void insertData(unsigned offset, const String&, ExceptionCode&, bool canShowLastCharacterIfSecure = false);

From Niwa: Nit: I'm not sure if canShowLastCharacterIfSecure is the right name. We certainly show last character when it's not secure.  Maybe canShowSecureLastCharacter?

From Kenneth: Ah I see you use MomentarilyRevealLastCharacter above.

Maybe, enum SecureMode { AlwaysHideCharacters, MomentarilyRevealLastCharacter }

Chang: I will use enum then.

> Source/WebCore/rendering/RenderText.cpp:1544
> +        secureLastCharacterTimer = new Timer<RenderText>(this, &RenderText::secureLastCharacter);
> +        gSecureLastCharacterTimers->add(this, secureLastCharacterTimer);

Mn... what happens if Timer fires after RenderText is destroyed?  Or is there an guarantee that it won't happen?

Chang: Good question. I will stop timer in the RenderText destructor.

> LayoutTests/editing/input/secure-text.html:65
> +    if (testIdx >= 0) {
> +        expectedSecureTextLen = tests[testIdx][2];
> +        textInputController.doCommand("moveForward:");
> +        assert(tests[testIdx][4], window.find(secureText(expectedSecureTextLen), false, true), "secured after delay.");
> +    }

It'll be nice if you could put this verification in a closure or something so that the verification code appears after the test code.  You can have two functions test & verify and make them mutually "recursive" via setTimeouts.

Chang: Can you show me details? I have to do two verifications, one before timeout and one after timeout.

> LayoutTests/editing/input/secure-text.html:78
> +    for(var i = 0; i < charSequence.length - 1; i++) {

Nit: space between for and (

Chang: Sure.

> LayoutTests/editing/input/secure-text.html:87
> +    window.setTimeout(run, 1200);

1200 seems like an awfully long time.  Can we shorten it to something like 500?  Or will that break the test / make the test flaky?

Chang: The reveal password time period is hardcoded to 1 sec (it is 2 seconds in iOs code!). So 1200ms is almost the minimum. I am thinking add a setting for this so in the layout test, we can set it to a much smaller value. Besides, different platforms may want different values based on their user experience. But I will do this in a separate patch.

> LayoutTests/editing/input/secure-text.html:96
> +    if (window.layoutTestController) {
> +        layoutTestController.dumpAsText();
> +        layoutTestController.waitUntilDone();
> +        run();
> +    }

Note that textInputController isn't implemented on all platforms so you might want to check window.textInputController and print out appropriate message here.

Chang: Sure. I will add the sanity check.

------- Comment #68 From Kenneth Rohde Christiansen 2011-01-17 15:41:04 PST (-) [reply] -------

(From update of attachment 79219 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=79219&action=review

> Source/WebCore/rendering/RenderText.h:122
> +    void secureLastCharacter(Timer<RenderText> * aTimer);

There should be no space before *, also you can remove aTimer.

Chang: will do.
Comment 70 Ryosuke Niwa 2011-01-18 07:45:04 PST
(In reply to comment #69)
> > LayoutTests/editing/input/secure-text.html:87
> > +    window.setTimeout(run, 1200);
> 
> 1200 seems like an awfully long time.  Can we shorten it to something like 500?  Or will that break the test / make the test flaky?
> 
> Chang: The reveal password time period is hardcoded to 1 sec (it is 2 seconds in iOs code!). So 1200ms is almost the minimum. I am thinking add a setting for this so in the layout test, we can set it to a much smaller value. Besides, different platforms may want different values based on their user experience. But I will do this in a separate patch.

Yes, we definitely want to be able to adjust the time interval.  I'd imagine some users (e.g. people with eyesight problems) want to adjust it to be a longer period of time.
Comment 71 Chang Shu 2011-01-19 12:09:37 PST
Created attachment 79460 [details]
fix patch 2

2nd patch after 1st round review.
Comment 72 Mike Fenton 2011-01-20 08:21:59 PST
This is a really useful patch for virtual keyboards.

Would it be possible to make it more configurable and driven by the editor by adding an editor function delayBeforeSecureLastCharacter() which returns the length of time to unmask the character for?  A value of 0 could then be used to immediately mask the character.

This would allow the timing to be tuned based on individual fields as well as the state of the device.  Devices that have multiple input methods may only want to unmask for the virtual keyboard, not in all cases.
Comment 73 Chang Shu 2011-01-20 08:48:52 PST
(In reply to comment #72)
> This is a really useful patch for virtual keyboards.
> 
> Would it be possible to make it more configurable and driven by the editor by adding an editor function delayBeforeSecureLastCharacter() which returns the length of time to unmask the character for?  A value of 0 could then be used to immediately mask the character.
> 
> This would allow the timing to be tuned based on individual fields as well as the state of the device.  Devices that have multiple input methods may only want to unmask for the virtual keyboard, not in all cases.

Yes, yes, I definitely want to do that (see previous comments above). I plan to add this setting in a separate patch so not to make this one too complicated.
Comment 74 Chang Shu 2011-01-24 09:56:16 PST
ping reviewer. thanks!
Comment 75 Eric Seidel 2011-01-28 12:17:28 PST
Comment on attachment 79460 [details]
fix patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=79460&action=review

Are we sure adding a destructor to RenderText isn't a perf hit?  I'm not sure if it is or not.

> Source/JavaScriptCore/wtf/text/WTFString.h:216
> +    void makeSecure(UChar aChar, bool shouldSecureLastCharacter)

Why shouldn't this take an enum like the StringImpl one does?  We could even move the enum onto WTFString if that's needed.

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:52
> +    CharacterData::SecureMode secureMode = (m_node->document()->frame() && m_node->document()->frame()->editor()->ignoreCompositionSelectionChange()) ? CharacterData::MomentarilyRevealLastCharacter : CharacterData::AlwaysHideCharacters;

Seems this needs a helper function.
Comment 76 Chang Shu 2011-01-28 13:58:50 PST
Created attachment 80492 [details]
fix patch 3

Updated patch based on Eric's comments.
Comment 77 Kenneth Rohde Christiansen 2011-01-29 15:15:09 PST
Comment on attachment 80492 [details]
fix patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=80492&action=review

> Source/WebCore/rendering/RenderText.cpp:56
> +static const float revealLastCharacterDurationInSeconds = 1.0f;

I believe that our style dictates to not use 1.0f but just 1. Could you check?
Comment 78 Chang Shu 2011-02-04 06:32:27 PST
> > +static const float revealLastCharacterDurationInSeconds = 1.0f;
> 
> I believe that our style dictates to not use 1.0f but just 1. Could you check?

You mean the webkit style check? The style bubble above is green.
Comment 79 Janne Koskinen 2011-02-07 02:17:10 PST
(In reply to comment #78)
> > > +static const float revealLastCharacterDurationInSeconds = 1.0f;
> > 
> > I believe that our style dictates to not use 1.0f but just 1. Could you check?
> 
> You mean the webkit style check? The style bubble above is green.

I don't think it is in check-webkit style, but indeed it is good style.
This proactively prevents erroneous statement like
>static const double foo = 1.0f; // oops
if precision is changed later on.
Comment 80 Chang Shu 2011-02-07 06:25:29 PST
> I don't think it is in check-webkit style, but indeed it is good style.
> This proactively prevents erroneous statement like
> >static const double foo = 1.0f; // oops
> if precision is changed later on.

Sure. I will make the change along with other changes if necessary (ping reviewers) or do it in the patch that supports configurable echo timeout.
Comment 81 Suresh Voruganti 2011-02-14 07:09:56 PST
Adding to Qtwebkit 2.1 Master bug
Comment 82 Andreas Kling 2011-02-17 06:44:04 PST
@Ryosuke: Could you have a look at this? It feels like something bordering on your alley :)
Comment 83 Ryosuke Niwa 2011-02-17 07:05:40 PST
(In reply to comment #82)
> @Ryosuke: Could you have a look at this? It feels like something bordering on your alley :)

I wish I could but I can't review RenderText change because I'm not familiar with the rendering engine.  I'd imagine eseidel, darin, or mitz would be able to review this patch.
Comment 84 Ademar Reis 2011-02-21 13:11:05 PST
Patch 3 added to qtwebkit-2.1 branch as 45527cd4cb9e2d7c00da2c72ae9f04172e36a4b3. This bug is now part of the "pending trunk inclusion" set.
Comment 85 Hajime Morrita 2011-04-19 13:31:59 PDT
Comment on attachment 80492 [details]
fix patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=80492&action=review

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:35
> +static CharacterData::SecureMode getTextNodeSecureMode(PassRefPtr<Text> node)

This logic could be a part of CharacterData class. How about to move this to CharacterData and add CharacterData:insertSecurableData() or something?

> Source/WebCore/rendering/RenderText.h:188
>  };

Is it possible to push down these to some new subclass like RenderSecureText?
Almost all RenderText objects are never edited and having special securely-editable class might be good separation.
I'm not sure if this is really good idea though. This is just an idea and you can freely ignore...
Comment 86 Alexey Proskuryakov 2011-04-19 13:51:25 PDT
I think that "secure mode" may not be a good name, as it collides with a different "secure input mode" that still lives in WebCore/SecureTextInput.h for Chromium (but should be in WebKit/chromium).

Something more straightforward like "drawsWithBullets" could work, except that the character is configurable.

> +static CharacterData::SecureMode getTextNodeSecureMode(PassRefPtr<Text> node)

I didn't read the patch itself, but this caught my eye as a coding style violation. Functions that return a value directly don't have a "get" prefix in WebKit.
Comment 87 Alexey Proskuryakov 2011-04-19 13:55:26 PDT
Also, with over 80 comments in the bug, I can't remember why we decided that this needs WTF and WebCore changes, and can't be done by the WebKit client.

The existing WTF::String::makeSecure() feels like an incredibly ugly layering violation to me. Perhaps the best approach would be to clean up the code first.
Comment 88 Chang Shu 2011-04-19 14:09:00 PDT
(In reply to comment #87)
> Also, with over 80 comments in the bug, I can't remember why we decided that this needs WTF and WebCore changes, and can't be done by the WebKit client.

Even I need refresh my memory. I think the changes over there is just to reuse the String::makeSecure code.
> 
> The existing WTF::String::makeSecure() feels like an incredibly ugly layering violation to me. Perhaps the best approach would be to clean up the code first.

Right. We should do this first. Over all, I feel the implementation has some flaws. For example, the echoing only happens on the last char. If you insert/delete chars in the middle, no echoing happens.

Good that this patch is drawing some attention again. I will find time work on it.
Comment 89 Eric Seidel 2011-05-23 13:28:44 PDT
Comment on attachment 80492 [details]
fix patch 3

The last comment was over a month ago.  Looks like Chang plans to revise this anyway.  Marking r-.
Comment 90 Chang Shu 2011-05-23 13:55:29 PDT
(In reply to comment #89)
> (From update of attachment 80492 [details])
> The last comment was over a month ago.  Looks like Chang plans to revise this anyway.  Marking r-.

Yes. I will revisit this bug when I have time. For now, the patch should be out-of-date anyway. Thanks.
Comment 91 Chang Shu 2011-06-07 07:10:42 PDT
Created attachment 96239 [details]
fix patch 3: rebaseline + minor changes
Comment 92 Chang Shu 2011-06-07 07:12:59 PDT
(In reply to comment #91)
> Created an attachment (id=96239) [details]
> fix patch 3: rebaseline + minor changes

This patch is NOT for review but for cherry-picking.
Comment 93 Antonio Gomes 2011-06-07 08:22:27 PDT
(In reply to comment #91)
> Created an attachment (id=96239) [details]
> fix patch 3: rebaseline + minor changes

Plans to put it up for revieW?
Comment 94 Chang Shu 2011-06-07 08:32:42 PDT
Comment on attachment 96239 [details]
fix patch 3: rebaseline + minor changes

>> fix patch 3: rebaseline + minor changes
> Plans to put it up for revieW?

Ok, why not give it a shot? :)
In the new patch, I removed the change in JavascriptCore so it doesn't have to depend on the cleanup of String::makeSecure.
In terms of WebKit vs. WebCore approach, I think the latter provides a cross-platform solution which will benefit all ports.
Comment 95 Antonio Gomes 2011-06-07 09:48:35 PDT
Comment on attachment 96239 [details]
fix patch 3: rebaseline + minor changes

View in context: https://bugs.webkit.org/attachment.cgi?id=96239&action=review

> Source/WebCore/rendering/RenderText.h:121
> +    void momentarilyRevealLastCharacter();

Is this for the last char in the text OR last typed char?

Like if you write some text, then move cursor to a different position other than the last position, and type some thing, does it work? If so, it sounds like the method name could be better?
Comment 96 Chang Shu 2011-06-07 10:39:42 PDT
(In reply to comment #95)
> (From update of attachment 96239 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96239&action=review
> 
> > Source/WebCore/rendering/RenderText.h:121
> > +    void momentarilyRevealLastCharacter();
> 
> Is this for the last char in the text OR last typed char?
> 
> Like if you write some text, then move cursor to a different position other than the last position, and type some thing, does it work? If so, it sounds like the method name could be better?

No, it won't work. What revealed is the last char in the text. I understand it's less desired behavior. At the time, I just tried to match the iOS behavior. It would be good to fix it with probably some big changes to the current implementation.
Comment 97 Mike Fenton 2011-06-07 10:46:02 PDT
(In reply to comment #96)
> > Like if you write some text, then move cursor to a different position other than the last position, and type some thing, does it work? If so, it sounds like the method name could be better?
> 
> No, it won't work. What revealed is the last char in the text. I understand it's less desired behavior. At the time, I just tried to match the iOS behavior. It would be good to fix it with probably some big changes to the current implementation.

If the input point isn't at the end of the text, should it reveal any characters at all then?

I'm also all for enhancing it to be the last character typed in the future.
Comment 98 Chang Shu 2011-06-07 11:09:35 PDT
(In reply to comment #97)
> (In reply to comment #96)
> > > Like if you write some text, then move cursor to a different position other than the last position, and type some thing, does it work? If so, it sounds like the method name could be better?
> > 
> > No, it won't work. What revealed is the last char in the text. I understand it's less desired behavior. At the time, I just tried to match the iOS behavior. It would be good to fix it with probably some big changes to the current implementation.
> 
> If the input point isn't at the end of the text, should it reveal any characters at all then?
> 
> I'm also all for enhancing it to be the last character typed in the future.

I prefer revealing the last typed char because the purpose of it is to give the user a confirmation(echo) of what just typed. But it's a rare case anyway.
Comment 99 Kenneth Rohde Christiansen 2011-06-07 11:37:05 PDT
pressing backspace and then revealing the last typed char, can be a security issue
Comment 100 Mike Fenton 2011-06-07 11:41:23 PDT
(In reply to comment #98)
> > If the input point isn't at the end of the text, should it reveal any characters at all then?
> > 
> > I'm also all for enhancing it to be the last character typed in the future.
> 
> I prefer revealing the last typed char because the purpose of it is to give the user a confirmation(echo) of what just typed. But it's a rare case anyway.

Yes, that's exactly what I was suggesting.  The last typed character should be revealed regardless of it's position in the text.
Comment 101 Chang Shu 2011-07-06 08:59:54 PDT
Created attachment 99840 [details]
patch 4

This patch echoes the last typed char and also does some code refactory.
Comment 102 WebKit Review Bot 2011-07-06 10:22:11 PDT
Comment on attachment 99840 [details]
patch 4

Attachment 99840 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8988789

New failing tests:
editing/input/secure-text.html
Comment 103 WebKit Review Bot 2011-07-06 10:22:19 PDT
Created attachment 99850 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 104 Chang Shu 2011-07-06 12:48:48 PDT
(In reply to comment #102)
> (From update of attachment 99840 [details])
> Attachment 99840 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8988789
> 
> New failing tests:
> editing/input/secure-text.html

The failure is due to a short timeout tolerance on chromium. I will add a setting to adjust password echo delay in the patch.
Comment 105 Chang Shu 2011-07-07 12:46:39 PDT
Created attachment 100021 [details]
patch 5

split tests to cut down the running time. The current tests are slow because the echo duration is hard-coded. I will work on a follow-up patch to make it configurable.
Comment 106 Alexey Proskuryakov 2011-07-08 14:26:16 PDT
Comment on attachment 100021 [details]
patch 5

View in context: https://bugs.webkit.org/attachment.cgi?id=100021&action=review

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:36
> +static CharacterData::SecureMode getTextNodeSecureMode(PassRefPtr<Text> node)

The argument should be a plain pointer, there is no ownership passing.

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:38
> +    return (node->document()->frame() && node->document()->frame()->editor()->ignoreCompositionSelectionChange()) ? CharacterData::MomentarilyRevealLastTypedCharacter : CharacterData::AlwaysHideCharacters;

I don't understand why ignoreCompositionSelectionChange() is a indicator of whether the last character should be revealed.

> Source/WebCore/rendering/RenderText.cpp:55
> +static const float revealLastTypedCharacterDurationInSeconds = 0.5f;

startOneShot() takes a double argument - why is this a float?

Also, "static" doesn't mean anything with "const" in C++.

> Source/WebCore/rendering/RenderText.cpp:207
> +            secureLastTypedCharacterTimer->stop();
> +            delete secureLastTypedCharacterTimer;

No need to stop a timer before deleting it.

> Source/WebCore/rendering/RenderText.h:178
> +    unsigned m_lastTypedCharacterOffsetToReveal;

I don't know if it's OK to add an unsigned and a bool (bitfield) to all RenderTexts. I suspect that it can be bad for memory use.
Comment 107 Chang Shu 2011-07-08 14:37:35 PDT
> > Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:38
> > +    return (node->document()->frame() && node->document()->frame()->editor()->ignoreCompositionSelectionChange()) ? CharacterData::MomentarilyRevealLastTypedCharacter : CharacterData::AlwaysHideCharacters;
> 
> I don't understand why ignoreCompositionSelectionChange() is a indicator of whether the last character should be revealed.

It seems we can only figure out whether the current input is composition type through this function. 
I also plan to add a flag in page settings to enable password echo even for qwerty keyboard. E.g. iOS echoes password even for full keyboard.

> > Source/WebCore/rendering/RenderText.h:178
> > +    unsigned m_lastTypedCharacterOffsetToReveal;
> 
> I don't know if it's OK to add an unsigned and a bool (bitfield) to all RenderTexts. I suspect that it can be bad for memory use.

I can probably pass the offset to the timer. but that also involves more code in setText and setTextInternal. I will give it a try.

Thanks for the review.
Comment 108 Alexey Proskuryakov 2011-07-08 14:51:54 PDT
> It seems we can only figure out whether the current input is composition type through this function. 
> I also plan to add a flag in page settings to enable password echo even for qwerty keyboard. E.g. iOS echoes password even for full keyboard.

Right - what's surprising to me is that password input is somehow related to composition. Now that I look at this more, I also suspect that this code will trigger the timer when typing Japanese or Chinese.
Comment 109 Chang Shu 2011-07-08 17:12:52 PDT
(In reply to comment #108)
> > It seems we can only figure out whether the current input is composition type through this function. 
> > I also plan to add a flag in page settings to enable password echo even for qwerty keyboard. E.g. iOS echoes password even for full keyboard.
> 
> Right - what's surprising to me is that password input is somehow related to composition. Now that I look at this more, I also suspect that this code will trigger the timer when typing Japanese or Chinese.

It should not. Inside function CharacterData::insertData, it also checks if the text isSecure. I think Japanese or Chinese characters can never be password.
Comment 110 Chang Shu 2011-07-11 12:20:23 PDT
Created attachment 100346 [details]
patch 6
Comment 111 Antonio Gomes 2011-07-11 12:36:11 PDT
Comment on attachment 100346 [details]
patch 6

View in context: https://bugs.webkit.org/attachment.cgi?id=100346&action=review

I think we are very close. Some minor comments and nits.

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:36
> +static CharacterData::SecureMode getTextNodeSecureMode(Text* node)

name it secureModeForTextNode maybe.

> Source/WebCore/rendering/RenderText.cpp:72
> +        m_renderText->setText(m_renderText->text(), true);

what is true here? maybe add a /*XXX*/ comment?

> Source/WebCore/rendering/RenderText.cpp:1670
> +    if (!gSecureTextTimers)
> +        gSecureTextTimers = new SecureTextTimerMap;

Is it ok to leak this guy here?

> Source/WebCore/rendering/RenderText.h:118
> +    bool isSecure() { return style()->textSecurity() != TSNONE; }

const method?
Comment 112 Chang Shu 2011-07-11 13:08:05 PDT
> > Source/WebCore/rendering/RenderText.cpp:1670
> > +    if (!gSecureTextTimers)
> > +        gSecureTextTimers = new SecureTextTimerMap;
> 
> Is it ok to leak this guy here?

I think some existing code also leaks the global variable. We can do better to clean it up while the size of the map decreases to 0. But we may end up creating and deleting it too often.
Comment 113 Balazs Kelemen 2011-07-11 14:10:02 PDT
(In reply to comment #112)
> > > Source/WebCore/rendering/RenderText.cpp:1670
> > > +    if (!gSecureTextTimers)
> > > +        gSecureTextTimers = new SecureTextTimerMap;
> > 
> > Is it ok to leak this guy here?
> 
> I think some existing code also leaks the global variable. We can do better to clean it up while the size of the map decreases to 0. But we may end up creating and deleting it too often.

It is a policy in WebKit to use heap for globals and never dealloc them.  This makes us shutting down faster ;) The preferred way is to define the variable with the DEFINE_STATIC_LOCAL macro at function scope.
Comment 114 Chang Shu 2011-07-11 14:17:34 PDT
> > I think some existing code also leaks the global variable. We can do better to clean it up while the size of the map decreases to 0. But we may end up creating and deleting it too often.
> 
> It is a policy in WebKit to use heap for globals and never dealloc them.  This makes us shutting down faster ;) The preferred way is to define the variable with the DEFINE_STATIC_LOCAL macro at function scope.

Thanks for the clarification!
Comment 115 Chang Shu 2011-07-11 14:21:49 PDT
Created attachment 100363 [details]
patch 7: follow up Antonio's comments
Comment 116 Alexey Proskuryakov 2011-07-11 15:49:10 PDT
Comment on attachment 100363 [details]
patch 7: follow up Antonio's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=100363&action=review

> Source/WebCore/dom/CharacterData.cpp:112
> +        RenderText* renderText = toRenderText(renderer());
> +        if (renderText->isSecure())

What guarantees that it's a RenderText?

> Source/WebCore/rendering/RenderText.cpp:63
> +    SecureTextTimer(RenderText* renderText)
> +    : m_renderText(renderText), m_lastTypedCharacterOffset(0) { }

Wrong formatting - if it's more than one line, it should be formatted like regular code (each brach on its own line, 4 spaces more for initializer list).

> Source/WebCore/rendering/RenderText.cpp:75
> +        ASSERT(gSecureTextTimers->get(m_renderText));

IIRC there is a contains() function for this.
Comment 117 Chang Shu 2011-07-12 07:28:09 PDT
> > Source/WebCore/dom/CharacterData.cpp:112
> > +        RenderText* renderText = toRenderText(renderer());
> > +        if (renderText->isSecure())
> 
> What guarantees that it's a RenderText?

I think it's safe with the current code. The only two classes derive from CharacterData are Comment and Text and only Text has renderer. There is also existing code in CharacterData.cpp that casts renderer() to RenderText in updateRenderer().

> > Source/WebCore/rendering/RenderText.cpp:75
> > +        ASSERT(gSecureTextTimers->get(m_renderText));
> 
> IIRC there is a contains() function for this.

The input to contains() is the value. m_renderText is the key. But you are right that I probably don't need to save m_renderText since I can use find() and contains(). I had concern on the performance but the map should be really small.
Comment 118 Chang Shu 2011-07-12 07:56:34 PDT
> > > Source/WebCore/rendering/RenderText.cpp:75
> > > +        ASSERT(gSecureTextTimers->get(m_renderText));
> > 
> > IIRC there is a contains() function for this.
> 
> The input to contains() is the value. m_renderText is the key. But you are right that I probably don't need to save m_renderText since I can use find() and contains(). I had concern on the performance but the map should be really small.

Sorry, the above comments were wrong. I was looking at the Vector api. :(
Comment 119 Chang Shu 2011-07-12 11:33:14 PDT
Created attachment 100530 [details]
patch 8: addresses ap's review, etc.
Comment 120 Alexey Proskuryakov 2011-07-13 14:22:37 PDT
Comment on attachment 100530 [details]
patch 8: addresses ap's review, etc.

View in context: https://bugs.webkit.org/attachment.cgi?id=100530&action=review

> Source/WebCore/dom/CharacterData.cpp:114
> +    if ((secureMode == MomentarilyRevealLastTypedCharacter) && renderer()) {
> +        RenderText* renderText = toRenderText(renderer());
> +        if (renderText->isSecure())
> +            renderText->momentarilyRevealLastTypedCharacter(offset + data.length() - 1);
> +    }

I think that this would read better as:

 110    if (secureMode == MomentarilyRevealLastTypedCharacter) {
 111        RenderText* renderText = toRenderText(renderer());
 112        if (renderText && renderText->isSecure())
 113            renderText->momentarilyRevealLastTypedCharacter(offset + data.length() - 1);
 114    }

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:38
> +    return (node->document()->frame() && node->document()->frame()->editor()->ignoreCompositionSelectionChange()) ? CharacterData::MomentarilyRevealLastTypedCharacter : CharacterData::AlwaysHideCharacters;

I still don't understand how password input is related to composition. Seems like we should have a client call (or a preference) that specifies this policy directly.

> Source/WebCore/rendering/RenderText.cpp:65
> +    { }

These should be on separate lines.
Comment 121 Chang Shu 2011-07-13 17:27:48 PDT
> > Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:38
> > +    return (node->document()->frame() && node->document()->frame()->editor()->ignoreCompositionSelectionChange()) ? CharacterData::MomentarilyRevealLastTypedCharacter : CharacterData::AlwaysHideCharacters;
> 
> I still don't understand how password input is related to composition. Seems like we should have a client call (or a preference) that specifies this policy directly.
> 
Thanks for the fine tune. For the question above, I think the use case is that we'd like to reveal the last-typed character only when all three conditions are true:
1. it's a password field
2. the input method is composition type.
3. it's entering new character, not deleting.

The reason behind this is only when we entering password using composition input method, we need some degree of conformance of what we have typed. It's not necessary when we type password using full keyboard.

Note that when 1 is true and 2 is false, we mask the whole password. And also note that the current iOS behavior is different: it always reveal last-typed character without considering condition 2. We cannot do the same as iOS because the same code is used for desktop and exposing the last character is not preferred. I plan to add the page setting to allow user enable password echo even the input method is not composition. But it will be in a separate patch.

But I do agree that making the setting work without considering composition seems simplify the case. Then it seems we should land the setting patch first before landing this patch. Otherwise, we will enable password echo even for desktop browser.
Comment 122 Alexey Proskuryakov 2011-07-14 11:43:40 PDT
Comment on attachment 100530 [details]
patch 8: addresses ap's review, etc.

So, for each port it would be:
- iOS doesn't need to consider composition status;
- Mobile Qt does need that;
- Every other port never reveals the last character.

As you are saying that making that more explicit will simplify the logic, let's do that.
Comment 123 Antonio Gomes 2011-07-14 11:58:58 PDT
(In reply to comment #122)
> (From update of attachment 100530 [details])
> So, for each port it would be:
> - iOS doesn't need to consider composition status;

Does iOS momentarily reveal the last character typed even with a physical keyboard (paired over bluetooth)?
Comment 124 Kenneth Rohde Christiansen 2011-07-14 14:11:24 PDT
> Thanks for the fine tune. For the question above, I think the use case is that we'd like to reveal the last-typed character only when all three conditions are true:
> 1. it's a password field
> 2. the input method is composition type.
> 3. it's entering new character, not deleting.

I am not sure that I will agree to number 2. On the N950 for instance (or via bluotooth keyboard on the N9 when that is supported) you can very well input passwords without having compositing. I am not convinces that that should have a different behavior than when inputting via the virtual keyboard.
Comment 125 Chang Shu 2011-07-19 20:01:24 PDT
Thanks for the review and comments, guys. I finally get access to internet as I have been travelling these days in my vacation. I will follow ap's suggestion using an option to enable/disable password echo and this should resolve some conflicts and confusion from different needs and platforms.
Comment 126 Chang Shu 2011-08-18 08:28:39 PDT
Created attachment 104347 [details]
patch 9
Comment 127 Alexey Proskuryakov 2011-08-18 11:49:39 PDT
Comment on attachment 104347 [details]
patch 9

Looks good to me. But as discussed on IRC, the code to reveal the last typed character should be in InsertIntoTextNodeCommand::doApply or even higher up the stack, not in CharacterData::insertData.
Comment 128 Alexey Proskuryakov 2011-08-18 11:51:35 PDT
Comment on attachment 104347 [details]
patch 9

View in context: https://bugs.webkit.org/attachment.cgi?id=104347&action=review

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:58
> +    m_node->insertData(m_offset, m_text, ec, secureModeForTextNode(m_node.get()));

Also, it's not great to have ExceptionCode not be the last argument.

> Source/WebCore/rendering/RenderText.cpp:65
> +    { }

These should be on separate lines.
Comment 129 Chang Shu 2011-08-18 12:53:23 PDT
Created attachment 104387 [details]
patch 10
Comment 130 Alexey Proskuryakov 2011-08-18 14:34:28 PDT
Comment on attachment 104387 [details]
patch 10

View in context: https://bugs.webkit.org/attachment.cgi?id=104387&action=review

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:53
> +    if (m_node->document()->settings() && m_node->document()->settings()->passwordEchoEnabled() && m_node->renderer()) {

Is the renderer() check needed, given that we checked rendererIsEditable() above?

> Source/WebCore/rendering/RenderText.h:120
> +    void secureText(UChar mask);

This shouldn't be public, should it?
Comment 131 Chang Shu 2011-08-18 18:53:01 PDT
Created attachment 104434 [details]
patch 11: r+ed with minor changes
Comment 132 WebKit Review Bot 2011-08-18 19:20:14 PDT
Comment on attachment 104434 [details]
patch 11: r+ed with minor changes

Attachment 104434 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9426670

New failing tests:
editing/inserting/4875189-1.html
Comment 133 WebKit Review Bot 2011-08-18 20:25:21 PDT
Comment on attachment 104434 [details]
patch 11: r+ed with minor changes

Attachment 104434 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9438022

New failing tests:
editing/inserting/4875189-1.html
Comment 134 Chang Shu 2011-08-19 06:51:28 PDT
> New failing tests:
> editing/inserting/4875189-1.html

I don't get why the above test failed with my patch. I have tried to build the latest chromium with my patch on linux and it passed. Besides, there's not much difference between patch 11 and patch 10 but patch 10 passed chromium. Can anyone give me a clue? thanks!
Comment 135 Chang Shu 2011-08-19 07:51:54 PDT
Comment on attachment 104434 [details]
patch 11: r+ed with minor changes

from the buldbot, i see r93392 is red. retry my patch.
Comment 136 WebKit Review Bot 2011-08-19 08:58:25 PDT
Comment on attachment 104434 [details]
patch 11: r+ed with minor changes

Rejecting attachment 104434 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2

Last 500 characters of output:
 svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT

Regressions: Unexpected image mismatch : (5)
  fast/text/atsui-multiple-renderers.html = IMAGE
  fast/text/international/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE

Regressions: Unexpected DumpRenderTree crashes : (1)
  editing/inserting/4875189-1.html = CRASH



Full output: http://queues.webkit.org/results/9434376
Comment 137 WebKit Review Bot 2011-08-22 13:36:00 PDT
Comment on attachment 104434 [details]
patch 11: r+ed with minor changes

Rejecting attachment 104434 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2

Last 500 characters of output:
 svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT

Regressions: Unexpected image mismatch : (5)
  fast/text/atsui-multiple-renderers.html = IMAGE
  fast/text/international/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE

Regressions: Unexpected DumpRenderTree crashes : (1)
  editing/inserting/4875189-1.html = CRASH



Full output: http://queues.webkit.org/results/9467897
Comment 138 Chang Shu 2011-08-23 13:55:06 PDT
Created attachment 104908 [details]
patch 12: fix the crash on chromium

The root cause of the crash on chromium was some silly coding in Internals functions.
Comment 139 WebKit Review Bot 2011-08-23 16:31:57 PDT
Comment on attachment 104908 [details]
patch 12: fix the crash on chromium

Clearing flags on attachment: 104908

Committed r93656: <http://trac.webkit.org/changeset/93656>
Comment 140 WebKit Review Bot 2011-08-23 16:32:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 141 Ademar Reis 2011-08-25 12:24:00 PDT
Revision r93656 cherry-picked into qtwebkit-2.2 with commit 3502319 <http://gitorious.org/webkit/qtwebkit/commit/3502319>