Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7754019:
Enable the keyboard to show the popup keyboard for inputting accented characters. (Closed)
|
Created:
9 years, 3 months ago by mazda Modified:
9 years, 3 months ago CC:
chromium-reviews, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnable the keyboard to show the popup keyboard for inputting accented characters.
I added accented character data for French layout to demonstrate how the data is used in this implementation.
I'll add the data of alternative characters for several symbols and the data for US layout in the later CLs.
BUG=none
TEST=manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102345
Patch Set 1 #
Total comments: 26
Patch Set 2 : 'Review fix' #
Total comments: 2
Patch Set 3 : Review fix #
Total comments: 14
Patch Set 4 : address comments #
Messages
Total messages: 15 (0 generated)
Can you send me some screenshots of how this looks? Thanks, Bryan On Thu, Sep 8, 2011 at 11:25 AM, <mazda@chromium.org> wrote: > Reviewers: bryeung, > > Description: > Enable the keyboard to show the popup keyboard for inputting accented > characters. > > I added accented character data for French layout to demonstrate how the > data is > used in this implementation. > I'll add the data of alternative characters for several symbols and the data > for > US layout in the later CLs. > > > BUG=none > TEST=manual > > > Please review this at http://codereview.chromium.org/7754019/ > > SVN Base: svn://svn.chromium.org/chrome/trunk/src > > Affected files: > M chrome/browser/resources/keyboard/common.js > M chrome/browser/resources/keyboard/index.html > M chrome/browser/resources/keyboard/layout_fr.js > A chrome/browser/resources/keyboard/layout_popup.js > M chrome/browser/resources/keyboard/layout_us.js > M chrome/browser/resources/keyboard/main.css > M chrome/browser/resources/keyboard/main.js > > >
http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... File chrome/browser/resources/keyboard/common.js (right): http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/common.js:255: // |keyLongHandler| is called. Where is the event being modified? Also, can we copy only the keys that we actually need into the new event? http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/common.js:298: if (evt.target != evt.currentTarget) { Why is this needed now? http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/common.js:334: * Dispatch custom events to the elements at the touch points. It's not clear to me why we need to make custom events here. http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/common.js:625: if (this.className_ == 'popupkey') { I don't like giving the class name this semantic value as well. Can we replace this with a different check? e.g. checking if there are any popupNames associated with any of the modes for this Key? http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... File chrome/browser/resources/keyboard/layout_fr.js (right): http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/layout_fr.js:10: new Key(C("a", "a", "fr_popup_a"), C("A", "A", "fr_popup_a"), C("1"), C("~")), Please try to stick to 80 characters. http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/layout_fr.js:50: new SvgKey("mic", " "), Please use single quotes. http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... File chrome/browser/resources/keyboard/layout_popup.js (right): http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/layout_popup.js:7: var KEYS_FR_POPUP_E = [ Is it possible to put the popup keyboards for a language in the same file as the rest of the definition for that language? I think that would make more sense. http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... File chrome/browser/resources/keyboard/main.js (right): http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/main.js:80: rowDiv.style.paddingRight = padding + '%'; Why is this necsesary? And why is it being done with a percentage? http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/main.js:144: element.addEventListener('mouseup', function(evt) { Is there a reason there is not also a touchend handler here?
http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... File chrome/browser/resources/keyboard/common.js (right): http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/common.js:255: // |keyLongHandler| is called. On 2011/09/08 19:22:35, bryeung wrote: > Where is the event being modified? > > Also, can we copy only the keys that we actually need into the new event? I'm not sure where it is modified, but evt.currentTarget is set to null before keyLongHandler is called. I think it's OK to copy a part of keys. Does 'the keys that we actually need' mean the key currently in use (evt.currentTarget) or all the key of Mouse Event including inherited ones? http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/common.js:298: if (evt.target != evt.currentTarget) { On 2011/09/08 19:22:35, bryeung wrote: > Why is this needed now? This kind of check should have been in the CL that added outHandler. Key element contains a div that holds text like this. <div class="key r1"> <div class="text-key">a</div> </div> There are three cases where mouseout handler is invoked. 1. When mouse cursor moves out of the internal div 2. When mouse cursor moves into the internal div 3. When mouse cursor moves out of the external div I intended to filter out case 1 and case 2 with this check. But I noticed while writing this comment that this check cannot handle case 2. I added a check for case 2. http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/common.js:334: * Dispatch custom events to the elements at the touch points. On 2011/09/08 19:22:35, bryeung wrote: > It's not clear to me why we need to make custom events here. touchmove and touchend events are not sent to the element located at touch points but to the element to which touchstart event is sent wherever the touch points move, so in order to highlight the keys located at the touch points, we need to somehow obtain the div elements corresponding to that keys. I think using custom event is handy rather than searching in the DOM tree. http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/common.js:625: if (this.className_ == 'popupkey') { On 2011/09/08 19:22:35, bryeung wrote: > I don't like giving the class name this semantic value as well. > > Can we replace this with a different check? e.g. checking if there are any > popupNames associated with any of the modes for this Key? popupNames are added for the keys that invokes the popup keyboard. This check is for the keys on the popup keyboard, so popupNames cannot be used for the check. Instead, I modified the code to create and use PopupKey class for the keys on the popup keyboard since the keys on the ordinary keyboard and the keys on the popup keyboard are quite different. Could you take another look at the code? http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... File chrome/browser/resources/keyboard/layout_fr.js (right): http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/layout_fr.js:10: new Key(C("a", "a", "fr_popup_a"), C("A", "A", "fr_popup_a"), C("1"), C("~")), On 2011/09/08 19:22:35, bryeung wrote: > Please try to stick to 80 characters. Done. http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/layout_fr.js:50: new SvgKey("mic", " "), On 2011/09/08 19:22:35, bryeung wrote: > Please use single quotes. Done. http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... File chrome/browser/resources/keyboard/layout_popup.js (right): http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/layout_popup.js:7: var KEYS_FR_POPUP_E = [ On 2011/09/08 19:22:35, bryeung wrote: > Is it possible to put the popup keyboards for a language in the same file as the > rest of the definition for that language? I think that would make more sense. Done. http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... File chrome/browser/resources/keyboard/main.js (right): http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/main.js:80: rowDiv.style.paddingRight = padding + '%'; On 2011/09/08 19:22:35, bryeung wrote: > Why is this necsesary? And why is it being done with a percentage? Keys occupy the entire row without these paddings since I'm using -webkit-box-flex to lay out the keys, and this looks weird when the popup keyboard has a row that contains few keys. Please look at the image of popup keyboard for e and i as examples that would look weird without the padding. A percentage is used for making the key width same as other rows. I plan to enable Row to set CSS class individually, and that would make this code unnecessary. http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/main.js:144: element.addEventListener('mouseup', function(evt) { On 2011/09/08 19:22:35, bryeung wrote: > Is there a reason there is not also a touchend handler here? This mouseup handler deals with the case that the mouse cursor has moved out of the element that has received mousedown event. On the other hand, touchend event is sent to and handled by the element that has received touchstart event even if the touch point have moved out of the element. That's why it is not necessary to add touchend handler here.
When you enable Row to set the CSS class individually, will the percentage padding code be removed? http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... File chrome/browser/resources/keyboard/common.js (right): http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/common.js:255: // |keyLongHandler| is called. On 2011/09/12 14:33:48, mazda wrote: > On 2011/09/08 19:22:35, bryeung wrote: > > Where is the event being modified? > > > > Also, can we copy only the keys that we actually need into the new event? > > I'm not sure where it is modified, but evt.currentTarget is set to null before > keyLongHandler is called. > I think it's OK to copy a part of keys. > Does 'the keys that we actually need' mean the key currently in use > (evt.currentTarget) or all the key of Mouse Event including inherited ones? Sorry. I think my use of the word "keys" was a poor choice. I meant: can we only copy the properties of the event that we will actually need into the object we are saving? http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/common.js:298: if (evt.target != evt.currentTarget) { On 2011/09/12 14:33:48, mazda wrote: > On 2011/09/08 19:22:35, bryeung wrote: > > Why is this needed now? > > This kind of check should have been in the CL that added outHandler. > > Key element contains a div that holds text like this. > > <div class="key r1"> > <div class="text-key">a</div> > </div> > > There are three cases where mouseout handler is invoked. > 1. When mouse cursor moves out of the internal div > 2. When mouse cursor moves into the internal div > 3. When mouse cursor moves out of the external div > > I intended to filter out case 1 and case 2 with this check. > But I noticed while writing this comment that this check cannot handle case 2. > I added a check for case 2. Thank you for the explanation. I think maybe we should add a comment to this code explaining why it is here. http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/common.js:334: * Dispatch custom events to the elements at the touch points. On 2011/09/12 14:33:48, mazda wrote: > On 2011/09/08 19:22:35, bryeung wrote: > > It's not clear to me why we need to make custom events here. > > touchmove and touchend events are not sent to the element located at touch > points but to the element to which touchstart event is sent wherever the touch > points move, so in order to highlight the keys located at the touch points, we > need to somehow obtain the div elements corresponding to that keys. I think > using custom event is handy rather than searching in the DOM tree. Is there a reason why we can't simulate regular events? Why can't we just send touchmove and touchend events? http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/common.js:625: if (this.className_ == 'popupkey') { On 2011/09/12 14:33:48, mazda wrote: > On 2011/09/08 19:22:35, bryeung wrote: > > I don't like giving the class name this semantic value as well. > > > > Can we replace this with a different check? e.g. checking if there are any > > popupNames associated with any of the modes for this Key? > > popupNames are added for the keys that invokes the popup keyboard. This check is > for the keys on the popup keyboard, so popupNames cannot be used for the check. > > Instead, I modified the code to create and use PopupKey class for the keys on > the popup keyboard since the keys on the ordinary keyboard and the keys on the > popup keyboard are quite different. > > Could you take another look at the code? I like this much better. Thank you. http://codereview.chromium.org/7754019/diff/8001/chrome/browser/resources/key... File chrome/browser/resources/keyboard/main.css (right): http://codereview.chromium.org/7754019/diff/8001/chrome/browser/resources/key... chrome/browser/resources/keyboard/main.css:74: background: rgba(30, 30, 30, 0.7); I think we should increase the opacity of the popup keyboards: I found them hard to see in the screenshots you provided.
> When you enable Row to set the CSS class individually, will the percentage > padding code be removed? Yes, I'll remove the padding code after I modify the Row later. http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... File chrome/browser/resources/keyboard/common.js (right): http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/common.js:255: // |keyLongHandler| is called. On 2011/09/13 23:53:04, bryeung wrote: > On 2011/09/12 14:33:48, mazda wrote: > > On 2011/09/08 19:22:35, bryeung wrote: > > > Where is the event being modified? > > > > > > Also, can we copy only the keys that we actually need into the new event? > > > > I'm not sure where it is modified, but evt.currentTarget is set to null before > > keyLongHandler is called. > > I think it's OK to copy a part of keys. > > Does 'the keys that we actually need' mean the key currently in use > > (evt.currentTarget) or all the key of Mouse Event including inherited ones? > > Sorry. I think my use of the word "keys" was a poor choice. > > I meant: can we only copy the properties of the event that we will actually need > into the object we are saving? Yes, that's fine. But if we use keyLongHandler for purpose case at some point, we need to remember to copy the properties that get additionally needed. I changed the code to copy only currentTarget for now. http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/common.js:298: if (evt.target != evt.currentTarget) { On 2011/09/13 23:53:04, bryeung wrote: > On 2011/09/12 14:33:48, mazda wrote: > > On 2011/09/08 19:22:35, bryeung wrote: > > > Why is this needed now? > > > > This kind of check should have been in the CL that added outHandler. > > > > Key element contains a div that holds text like this. > > > > <div class="key r1"> > > <div class="text-key">a</div> > > </div> > > > > There are three cases where mouseout handler is invoked. > > 1. When mouse cursor moves out of the internal div > > 2. When mouse cursor moves into the internal div > > 3. When mouse cursor moves out of the external div > > > > I intended to filter out case 1 and case 2 with this check. > > But I noticed while writing this comment that this check cannot handle case 2. > > I added a check for case 2. > > Thank you for the explanation. I think maybe we should add a comment to this > code explaining why it is here. Added a comment. http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/common.js:334: * Dispatch custom events to the elements at the touch points. On 2011/09/13 23:53:04, bryeung wrote: > On 2011/09/12 14:33:48, mazda wrote: > > On 2011/09/08 19:22:35, bryeung wrote: > > > It's not clear to me why we need to make custom events here. > > > > touchmove and touchend events are not sent to the element located at touch > > points but to the element to which touchstart event is sent wherever the touch > > points move, so in order to highlight the keys located at the touch points, we > > need to somehow obtain the div elements corresponding to that keys. I think > > using custom event is handy rather than searching in the DOM tree. > > Is there a reason why we can't simulate regular events? Why can't we just send > touchmove and touchend events? touchmove and touchend events are sent to the element that touchstart is sent, so they are not the events keys on the popup keyboard should handle. If we send touchmove or touchend here, we need to check whether the event is dispatched from here or ordinary touch event, and the event handler needs to change the behavior based on the event source. I do not think it is a good idea. http://codereview.chromium.org/7754019/diff/8001/chrome/browser/resources/key... File chrome/browser/resources/keyboard/main.css (right): http://codereview.chromium.org/7754019/diff/8001/chrome/browser/resources/key... chrome/browser/resources/keyboard/main.css:74: background: rgba(30, 30, 30, 0.7); On 2011/09/13 23:53:04, bryeung wrote: > I think we should increase the opacity of the popup keyboards: I found them hard > to see in the screenshots you provided. Tweaked the colors according to the recent color scheme change. I'll ask Alex to make the appearance better later.
Printscreen key didn't work for some reason in the recent image, so I have no screenshot, sorry. Main color changes are - increasing the opacity of the popup keyboard, and - changing the text color on the keys on the popup keyboard. Thanks, -Yasuhiro On Fri, Sep 16, 2011 at 10:16 PM, <mazda@chromium.org> wrote: > When you enable Row to set the CSS class individually, will the percentage >> padding code be removed? >> > Yes, I'll remove the padding code after I modify the Row later. > > > > http://codereview.chromium.**org/7754019/diff/1/chrome/** > browser/resources/keyboard/**common.js<http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboard/common.js> > File chrome/browser/resources/**keyboard/common.js (right): > > http://codereview.chromium.**org/7754019/diff/1/chrome/** > browser/resources/keyboard/**common.js#newcode255<http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboard/common.js#newcode255> > chrome/browser/resources/**keyboard/common.js:255: // |keyLongHandler| is > called. > On 2011/09/13 23:53:04, bryeung wrote: > >> On 2011/09/12 14:33:48, mazda wrote: >> > On 2011/09/08 19:22:35, bryeung wrote: >> > > Where is the event being modified? >> > > >> > > Also, can we copy only the keys that we actually need into the new >> > event? > >> > >> > I'm not sure where it is modified, but evt.currentTarget is set to >> > null before > >> > keyLongHandler is called. >> > I think it's OK to copy a part of keys. >> > Does 'the keys that we actually need' mean the key currently in use >> > (evt.currentTarget) or all the key of Mouse Event including >> > inherited ones? > > Sorry. I think my use of the word "keys" was a poor choice. >> > > I meant: can we only copy the properties of the event that we will >> > actually need > >> into the object we are saving? >> > > Yes, that's fine. But if we use keyLongHandler for purpose case at some > point, we need to remember to copy the properties that get additionally > needed. > I changed the code to copy only currentTarget for now. > > > http://codereview.chromium.**org/7754019/diff/1/chrome/** > browser/resources/keyboard/**common.js#newcode298<http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboard/common.js#newcode298> > chrome/browser/resources/**keyboard/common.js:298: if (evt.target != > evt.currentTarget) { > On 2011/09/13 23:53:04, bryeung wrote: > >> On 2011/09/12 14:33:48, mazda wrote: >> > On 2011/09/08 19:22:35, bryeung wrote: >> > > Why is this needed now? >> > >> > This kind of check should have been in the CL that added outHandler. >> > >> > Key element contains a div that holds text like this. >> > >> > <div class="key r1"> >> > <div class="text-key">a</div> >> > </div> >> > >> > There are three cases where mouseout handler is invoked. >> > 1. When mouse cursor moves out of the internal div >> > 2. When mouse cursor moves into the internal div >> > 3. When mouse cursor moves out of the external div >> > >> > I intended to filter out case 1 and case 2 with this check. >> > But I noticed while writing this comment that this check cannot >> > handle case 2. > >> > I added a check for case 2. >> > > Thank you for the explanation. I think maybe we should add a comment >> > to this > >> code explaining why it is here. >> > > Added a comment. > > > http://codereview.chromium.**org/7754019/diff/1/chrome/** > browser/resources/keyboard/**common.js#newcode334<http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboard/common.js#newcode334> > chrome/browser/resources/**keyboard/common.js:334: * Dispatch custom > events to the elements at the touch points. > On 2011/09/13 23:53:04, bryeung wrote: > >> On 2011/09/12 14:33:48, mazda wrote: >> > On 2011/09/08 19:22:35, bryeung wrote: >> > > It's not clear to me why we need to make custom events here. >> > >> > touchmove and touchend events are not sent to the element located at >> > touch > >> > points but to the element to which touchstart event is sent wherever >> > the touch > >> > points move, so in order to highlight the keys located at the touch >> > points, we > >> > need to somehow obtain the div elements corresponding to that keys. >> > I think > >> > using custom event is handy rather than searching in the DOM tree. >> > > Is there a reason why we can't simulate regular events? Why can't we >> > just send > >> touchmove and touchend events? >> > > touchmove and touchend events are sent to the element that touchstart is > sent, so they are not the events keys on the popup keyboard should > handle. If we send touchmove or touchend here, we need to check whether > the event is dispatched from here or ordinary touch event, and the event > handler needs to change the behavior based on the event source. > I do not think it is a good idea. > > > http://codereview.chromium.**org/7754019/diff/8001/chrome/** > browser/resources/keyboard/**main.css<http://codereview.chromium.org/7754019/diff/8001/chrome/browser/resources/keyboard/main.css> > File chrome/browser/resources/**keyboard/main.css (right): > > http://codereview.chromium.**org/7754019/diff/8001/chrome/** > browser/resources/keyboard/**main.css#newcode74<http://codereview.chromium.org/7754019/diff/8001/chrome/browser/resources/keyboard/main.css#newcode74> > chrome/browser/resources/**keyboard/main.css:74: background: rgba(30, 30, > 30, 0.7); > On 2011/09/13 23:53:04, bryeung wrote: > >> I think we should increase the opacity of the popup keyboards: I found >> > them hard > >> to see in the screenshots you provided. >> > > Tweaked the colors according to the recent color scheme change. > I'll ask Alex to make the appearance better later. > > > http://codereview.chromium.**org/7754019/<http://codereview.chromium.org/7754... >
On Fri, Sep 16, 2011 at 9:34 AM, Yasuhiro Matsuda <mazda@chromium.org>wrote: > Printscreen key didn't work for some reason in the recent image, > so I have no screenshot, sorry. > Main color changes are > - increasing the opacity of the popup keyboard, and > - changing the text color on the keys on the popup keyboard. > Have you also merged in kgr@s changes to the CSS? > > Thanks, > -Yasuhiro > > > On Fri, Sep 16, 2011 at 10:16 PM, <mazda@chromium.org> wrote: > >> When you enable Row to set the CSS class individually, will the >>> percentage >>> padding code be removed? >>> >> Yes, I'll remove the padding code after I modify the Row later. >> >> >> >> http://codereview.chromium.**org/7754019/diff/1/chrome/** >> browser/resources/keyboard/**common.js<http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboard/common.js> >> File chrome/browser/resources/**keyboard/common.js (right): >> >> http://codereview.chromium.**org/7754019/diff/1/chrome/** >> browser/resources/keyboard/**common.js#newcode255<http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboard/common.js#newcode255> >> chrome/browser/resources/**keyboard/common.js:255: // |keyLongHandler| is >> called. >> On 2011/09/13 23:53:04, bryeung wrote: >> >>> On 2011/09/12 14:33:48, mazda wrote: >>> > On 2011/09/08 19:22:35, bryeung wrote: >>> > > Where is the event being modified? >>> > > >>> > > Also, can we copy only the keys that we actually need into the new >>> >> event? >> >>> > >>> > I'm not sure where it is modified, but evt.currentTarget is set to >>> >> null before >> >>> > keyLongHandler is called. >>> > I think it's OK to copy a part of keys. >>> > Does 'the keys that we actually need' mean the key currently in use >>> > (evt.currentTarget) or all the key of Mouse Event including >>> >> inherited ones? >> >> Sorry. I think my use of the word "keys" was a poor choice. >>> >> >> I meant: can we only copy the properties of the event that we will >>> >> actually need >> >>> into the object we are saving? >>> >> >> Yes, that's fine. But if we use keyLongHandler for purpose case at some >> point, we need to remember to copy the properties that get additionally >> needed. >> I changed the code to copy only currentTarget for now. >> >> >> http://codereview.chromium.**org/7754019/diff/1/chrome/** >> browser/resources/keyboard/**common.js#newcode298<http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboard/common.js#newcode298> >> chrome/browser/resources/**keyboard/common.js:298: if (evt.target != >> evt.currentTarget) { >> On 2011/09/13 23:53:04, bryeung wrote: >> >>> On 2011/09/12 14:33:48, mazda wrote: >>> > On 2011/09/08 19:22:35, bryeung wrote: >>> > > Why is this needed now? >>> > >>> > This kind of check should have been in the CL that added outHandler. >>> > >>> > Key element contains a div that holds text like this. >>> > >>> > <div class="key r1"> >>> > <div class="text-key">a</div> >>> > </div> >>> > >>> > There are three cases where mouseout handler is invoked. >>> > 1. When mouse cursor moves out of the internal div >>> > 2. When mouse cursor moves into the internal div >>> > 3. When mouse cursor moves out of the external div >>> > >>> > I intended to filter out case 1 and case 2 with this check. >>> > But I noticed while writing this comment that this check cannot >>> >> handle case 2. >> >>> > I added a check for case 2. >>> >> >> Thank you for the explanation. I think maybe we should add a comment >>> >> to this >> >>> code explaining why it is here. >>> >> >> Added a comment. >> >> >> http://codereview.chromium.**org/7754019/diff/1/chrome/** >> browser/resources/keyboard/**common.js#newcode334<http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboard/common.js#newcode334> >> chrome/browser/resources/**keyboard/common.js:334: * Dispatch custom >> events to the elements at the touch points. >> On 2011/09/13 23:53:04, bryeung wrote: >> >>> On 2011/09/12 14:33:48, mazda wrote: >>> > On 2011/09/08 19:22:35, bryeung wrote: >>> > > It's not clear to me why we need to make custom events here. >>> > >>> > touchmove and touchend events are not sent to the element located at >>> >> touch >> >>> > points but to the element to which touchstart event is sent wherever >>> >> the touch >> >>> > points move, so in order to highlight the keys located at the touch >>> >> points, we >> >>> > need to somehow obtain the div elements corresponding to that keys. >>> >> I think >> >>> > using custom event is handy rather than searching in the DOM tree. >>> >> >> Is there a reason why we can't simulate regular events? Why can't we >>> >> just send >> >>> touchmove and touchend events? >>> >> >> touchmove and touchend events are sent to the element that touchstart is >> sent, so they are not the events keys on the popup keyboard should >> handle. If we send touchmove or touchend here, we need to check whether >> the event is dispatched from here or ordinary touch event, and the event >> handler needs to change the behavior based on the event source. >> I do not think it is a good idea. >> >> >> http://codereview.chromium.**org/7754019/diff/8001/chrome/** >> browser/resources/keyboard/**main.css<http://codereview.chromium.org/7754019/diff/8001/chrome/browser/resources/keyboard/main.css> >> File chrome/browser/resources/**keyboard/main.css (right): >> >> http://codereview.chromium.**org/7754019/diff/8001/chrome/** >> browser/resources/keyboard/**main.css#newcode74<http://codereview.chromium.org/7754019/diff/8001/chrome/browser/resources/keyboard/main.css#newcode74> >> chrome/browser/resources/**keyboard/main.css:74: background: rgba(30, 30, >> 30, 0.7); >> On 2011/09/13 23:53:04, bryeung wrote: >> >>> I think we should increase the opacity of the popup keyboards: I found >>> >> them hard >> >>> to see in the screenshots you provided. >>> >> >> Tweaked the colors according to the recent color scheme change. >> I'll ask Alex to make the appearance better later. >> >> >> http://codereview.chromium.**org/7754019/<http://codereview.chromium.org/7754... >> > >
I don't think I'm seeing your most recent changes. Do you need to re-upload? http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... File chrome/browser/resources/keyboard/common.js (right): http://codereview.chromium.org/7754019/diff/1/chrome/browser/resources/keyboa... chrome/browser/resources/keyboard/common.js:255: // |keyLongHandler| is called. On 2011/09/16 13:16:37, mazda wrote: > I changed the code to copy only currentTarget for now. I don't see that change in this patch. Do you need to re-upload?
I reuploaded the changes, sorry. Please take another look. > Have you also merged in kgr@s changes to the CSS? Yes, I merged the CSS changes and tweaked my CSS change so that it becomes more viewable.
LGTM but please fix up the nits http://codereview.chromium.org/7754019/diff/20001/chrome/browser/resources/ke... File chrome/browser/resources/keyboard/common.js (right): http://codereview.chromium.org/7754019/diff/20001/chrome/browser/resources/ke... chrome/browser/resources/keyboard/common.js:259: var evt2 = {}; nit: evtCopy instead of evt2? http://codereview.chromium.org/7754019/diff/20001/chrome/browser/resources/ke... chrome/browser/resources/keyboard/common.js:349: * touchpointmove events are dispatched responding to a touchmove and nit: make the name more obvious (e.g. touchmove_popup/touchend_popup, or something like that) http://codereview.chromium.org/7754019/diff/20001/chrome/browser/resources/ke... chrome/browser/resources/keyboard/common.js:354: function dispatchTouchPointEvent(evt) { nit: make the name more obvious (e.g. dispatchCustomPopupEvents) http://codereview.chromium.org/7754019/diff/20001/chrome/browser/resources/ke... chrome/browser/resources/keyboard/common.js:416: * @param {string} name The name of the popup keyboard. missing evt param doc http://codereview.chromium.org/7754019/diff/20001/chrome/browser/resources/ke... chrome/browser/resources/keyboard/common.js:482: function hidePopupKeyboard() { Is it worth moving the clean-up code (that removes child elements) to hide? Would that provide better performance when showing the popup keyboard? http://codereview.chromium.org/7754019/diff/20001/chrome/browser/resources/ke... File chrome/browser/resources/keyboard/main.css (right): http://codereview.chromium.org/7754019/diff/20001/chrome/browser/resources/ke... chrome/browser/resources/keyboard/main.css:119: color: #fff; sort please http://codereview.chromium.org/7754019/diff/20001/chrome/browser/resources/ke... chrome/browser/resources/keyboard/main.css:126: color: #fff; sort please
http://codereview.chromium.org/7754019/diff/20001/chrome/browser/resources/ke... File chrome/browser/resources/keyboard/common.js (right): http://codereview.chromium.org/7754019/diff/20001/chrome/browser/resources/ke... chrome/browser/resources/keyboard/common.js:259: var evt2 = {}; On 2011/09/20 19:30:50, bryeung wrote: > nit: evtCopy instead of evt2? Done. http://codereview.chromium.org/7754019/diff/20001/chrome/browser/resources/ke... chrome/browser/resources/keyboard/common.js:349: * touchpointmove events are dispatched responding to a touchmove and On 2011/09/20 19:30:50, bryeung wrote: > nit: make the name more obvious (e.g. touchmove_popup/touchend_popup, or > something like that) Done. http://codereview.chromium.org/7754019/diff/20001/chrome/browser/resources/ke... chrome/browser/resources/keyboard/common.js:354: function dispatchTouchPointEvent(evt) { On 2011/09/20 19:30:50, bryeung wrote: > nit: make the name more obvious (e.g. dispatchCustomPopupEvents) Done. http://codereview.chromium.org/7754019/diff/20001/chrome/browser/resources/ke... chrome/browser/resources/keyboard/common.js:416: * @param {string} name The name of the popup keyboard. On 2011/09/20 19:30:50, bryeung wrote: > missing evt param doc Done. http://codereview.chromium.org/7754019/diff/20001/chrome/browser/resources/ke... chrome/browser/resources/keyboard/common.js:482: function hidePopupKeyboard() { On 2011/09/20 19:30:50, bryeung wrote: > Is it worth moving the clean-up code (that removes child elements) to hide? > Would that provide better performance when showing the popup keyboard? That would provide better performance if the popup keyboard is shown again for another key. Probably such use cases are more frequent than the cases showing the popup key for one key consecutively, so I moved the clean-up code from showPopupKeyboard to hidePopupKeyboard. http://codereview.chromium.org/7754019/diff/20001/chrome/browser/resources/ke... File chrome/browser/resources/keyboard/main.css (right): http://codereview.chromium.org/7754019/diff/20001/chrome/browser/resources/ke... chrome/browser/resources/keyboard/main.css:119: color: #fff; On 2011/09/20 19:30:50, bryeung wrote: > sort please Done. http://codereview.chromium.org/7754019/diff/20001/chrome/browser/resources/ke... chrome/browser/resources/keyboard/main.css:126: color: #fff; On 2011/09/20 19:30:50, bryeung wrote: > sort please Done.
CQ is trying tha patch. Follow status at https://chromium-status.appspot.com/cq/receiver/mazda%40chromium.org/7754019/...
lgtm
Change committed as 102345 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
