Closed Bug 1040621 Opened 10 years ago Closed 10 years ago

Set lang attribute to the layout containing div

Categories

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::Keyboard
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
2.1 S1 (1aug)

People

(Reporter: timdream, Assigned: anindyapandey, Mentored)

References

Details

(Whiteboard: [good first bug][p=1][mentor-lang=zh])

Attachments

(1 file, 2 obsolete files)

[gaia] timdream:layout-lang > mozilla-b2g:master
(deleted), text/x-github-pull-request
timdream
: review+
Details
The browser selects the best font to use based on the lang attribute of the DOM element. We didn't set it so it currently always use the lang of <html>, which is the user UI language. This does not make and significant different except for for CJK where different style of glyphs occupies the same Unicode code point a lot. (Actually, we only ship one CJK font in the phone for now, so the patch here might not make any difference at all, yet.)
Converting this bug into a good first bug. The taker of this bug is expected to be able to test and run Gaia and inspect the DOM of keyboard app to verify the fix. The fix should add a |lang| field to every file in apps/keyboard/js/imes/*.js, and render.js will pick up that and set the lang attribute to the <div> accordingly. As we are already using lang code for many of the layouts, maybe it make sense to do like "if there is no lang property, use the layout name" logic.
Assignee: timdream → nobody
Mentor: timdream
Whiteboard: [p=1] → [good first bug][p=1][mentor-lang=zh]
Status: ASSIGNED → NEW
Hello, Is this bug assigned to anyone. I would love to take this as my first bug but i will require a lot of mentoring?
Hi, You are very welcome to try to work on this bug. Here is the code and docs to start working on Gaia. https://github.com/mozilla-b2g/gaia https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia The code of the keyboard app is in apps/keyboard https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard Just comment on this bug if you have any questions.
Assignee: nobody → talhaanwar.anwar
Status: NEW → ASSIGNED
Flags: needinfo?(talhaanwar.anwar)
I am ready to take this. Thank you for mentoring me on this.
Flags: needinfo?(talhaanwar.anwar)
Some idea on this bug: 1. Let's put a |lang| attribute on every layout def in apps/keyboard/js/layouts/ 2. It will get loaded and switched to by layoutManager as currentLayout, but we need to intelligently make sure the property got property copied to currentModifiedLayout (in preparation of bug 1046001; no one should use information in currentLayout anymore) 3. Since the currentModifiedLayout is passed in IMERender.draw(), render.js can get that and set the lang attribute on the containing DOM element.
Hey +Tim, Thank you for mentoring me. I fear that I don't understand what you meant by "Let's put a |lang| attribute on every layout def in apps/keyboard/js/layouts/" . I understand the lang attribute of the DOM element (I didn't know it,I learnt it today) but i don't understand what you mean by layout def in apps/keyboard/js/layouts/ (please help me identify it). Secondly, I don't understand but we need to intelligently make sure the property got property copied to currentModifiedLayout. Thank you.
(In reply to talhaanwar.anwar from comment #6) > Hey +Tim, > > Thank you for mentoring me. > > I fear that I don't understand what you meant by "Let's put a |lang| > attribute on every layout def in apps/keyboard/js/layouts/" . In FxOS Keyboard, each of the keyboard are defined by a JS file located in that path. You can add a |lang| property in the object of each of these files so render.js can pick it up. > I understand > the lang attribute of the DOM element (I didn't know it,I learnt it today) > but i don't understand what you mean by layout def in > apps/keyboard/js/layouts/ (please help me identify it). Secondly, I don't > understand but we need to intelligently make sure the property got property > copied to currentModifiedLayout. > I was just thinking aloud here. Given the fact we are actually giving each of the layouts the id that we put in the lang property (e.g. fr.js contains layout for lang="fr"), we can actually omit add that property and have render.js figure out the lang code to use by the id of the layout. That however only works for some layout since a lot of layouts comes with suffixes that doesn't belong to the lang tag (e.g. "en-Neo"). If you can't figure out this part, maybe it's fine and more explicit to add lang property to all layout scripts. Explicit API is better sometimes.
How's everyhing? Anything I can help?
Flags: needinfo?(talhaanwar.anwar)
Hey +Tim. Sorry for the late reply. I have figured out what to do but I am having problem in building Firefox. I am trying and will get back to you real soon.
Flags: needinfo?(talhaanwar.anwar)
Anwar, Are you working on this? Mind if i take this?
Flags: needinfo?(talhaanwar.anwar)
This is pretty old already .. Can I assign this to myself and fix it ?
Unfortunately this bug is unfit for first-time contributor now due to refactoring work like bug 1105178 etc. Rudy will know when it's good for people to be involved.
Assignee: talhaanwar.anwar → nobody
Mentor: rlu
Flags: needinfo?(talhaanwar.anwar) → needinfo?(rlu)
Whiteboard: [good first bug][p=1][mentor-lang=zh] → [p=1][mentor-lang=zh]
I think we would start working on this once bug 1115247 is resolved. Thanks.
Depends on: 1115247
Flags: needinfo?(rlu)
Now that bug 1115247 is resolved with the 1st phase of rendering refactoring, we can continue to work on this bug. Mark it as a good first bug.
Whiteboard: [p=1][mentor-lang=zh] → [good first bug][p=1][mentor-lang=zh]
Anindya-Pandey would like to work on this. Please noted that I am currently landing a build test in bug 1029951 -- since the test would assert the size of the layout scripts, changes made here will make the tests fail. Please run and amend the assertion accordingly when you patch the code.
Assignee: nobody → anindyapandey
make[1]: ***[build-test-integration] Error 1 this is the error I get everytime I try to run the build test.
Flags: needinfo?(timdream)
(In reply to Anindya-Pandey from comment #16) > make[1]: ***[build-test-integration] Error 1 > this is the error I get everytime I try to run the build test. I don't have enough information to say what's missing; can you attach the full log?
Flags: needinfo?(timdream)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #17) > (In reply to Anindya-Pandey from comment #16) > > make[1]: ***[build-test-integration] Error 1 > > this is the error I get everytime I try to run the build test. > > I don't have enough information to say what's missing; can you attach the > full log? anindya@anindya-virtual-machine:~/gaia$ make build-test-integration TEST_FILES=apps/keyboard/test/build/integration/keyboard_test.js Test SDK directory: /home/anindya/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01 test -f /home/anindya/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01/b2g/xpcshell run-js-command gaia/build-test [cmd] mocha --harmony --reporter spec --ui tdd --timeout 600000 /home/anindya/gaia/apps/keyboard/test/build/integration/keyboard_test.js make: *** [build-test-integration] Error 1
Flags: needinfo?(timdream)
Could you do make really-clean and run the command again and paste the output? Something must be wrong during the environment setup. Here is mine after really-clean: ==== $ make build-test-integration TEST_FILES=apps/keyboard/test/build/integration/keyboard_test.js Test SDK directory: /Users/timdream/Repositories/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01 rm -rf /Users/timdream/Repositories/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01 mkdir -p "/Users/timdream/Repositories/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01" Downloading B2G SDK... /usr/bin/curl -OLS "http://ftp.mozilla.org/pub/mozilla.org/b2g/nightly/2014/08/2014-08-12-04-02-01-mozilla-central/b2g-34.0a1.multi.mac64.dmg" % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 72.5M 100 72.5M 0 0 481k 0 0:02:34 0:02:34 --:--:-- 916k hdiutil attach b2g-34.0a1.multi.mac64.dmg -readonly -nobrowse -mount required -mountpoint .b2g.tmp Checksumming Driver Descriptor Map (DDM : 0)… Driver Descriptor Map (DDM : 0): verified CRC32 $939A494A Checksumming Apple (Apple_partition_map : 1)… Apple (Apple_partition_map : 1): verified CRC32 $8C0B7D7D Checksumming DiscRecording 6.0d1 (Apple_HFS : 2)… ............................................................................... DiscRecording 6.0d1 (Apple_HFS : 2): verified CRC32 $8A1F6131 verified CRC32 $7AF48C60 /dev/disk2 Apple_partition_scheme /dev/disk2s1 Apple_partition_map /dev/disk2s2 Apple_HFS /Users/timdream/Repositories/gaia/.b2g.tmp cp -Rf .b2g.tmp/* "/Users/timdream/Repositories/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01" ln -sf "/Users/timdream/Repositories/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01/B2G.app/Contents/MacOS" "/Users/timdream/Repositories/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01/b2g" umount .b2g.tmp test -f /Users/timdream/Repositories/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01/B2G.app/Contents/MacOS/xpcshell # Running make without using a dependency ensures that we can run # "make node_modules" with a custom NODE_MODULES_GIT_URL variable, and then # run another target without specifying the variable /Applications/Xcode.app/Contents/Developer/usr/bin/make modules.tar Downloading latest node_modules package. This may take several minutes... /usr/bin/curl -OLS https://github.com/mozilla-b2g/gaia-node-modules/tarball/d22a9100fd46b8387eb4a992d8378166fc5d1a5b &&\ mv d22a9100fd46b8387eb4a992d8378166fc5d1a5b "modules.tar" % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 178 0 178 0 0 58 0 --:--:-- 0:00:03 --:--:-- 58 100 31.3M 100 31.3M 0 0 515k 0 0:01:02 0:01:02 --:--:-- 1656k tar --strip-components 1 -x -m -f modules.tar "mozilla-b2g-gaia-node-modules-*/node_modules" tar: Failed to set default locale npm install && npm rebuild (... omitted ...) node_modules installed. touch -c node_modules run-js-command gaia/build-test [cmd] mocha --harmony --reporter spec --ui tdd --timeout 600000 /Users/timdream/Repositories/gaia/apps/keyboard/test/build/integration/keyboard_test.js (... test output omitted ...) === BTW, if download takes too much time, you can show the progress just like me, by removing |-nv| option from WGET_OPTS. I removed the |s| option of curl instead since I am on a Mac: make build-test-integration WGET_OPTS="-c" TEST_FILES=apps/keyboard/test/build/integration/keyboard_test.js
Flags: needinfo?(timdream)
anindya@anindya-virtual-machine:~/gaia$ make really-clean rm -rf profile profile-debug profile-test profile-gaia-test-b2g profile-gaia-test-firefox profile-raptor profile /home/anindya/gaia/build_stage docs minidumps rm -rf b2g-* .b2g-* b2g_sdk node_modules b2g modules.tar js-marionette-env anindya@anindya-virtual-machine:~/gaia$ make build-test-integration TEST_FILES=apps/keyboard/test/build/integration/keyboard_test.js Test SDK directory: /home/anindya/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01 rm -rf /home/anindya/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01 mkdir -p "/home/anindya/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01" Downloading B2G SDK... wget -c -nv "http://ftp.mozilla.org/pub/mozilla.org/b2g/nightly/2014/08/2014-08-12-04-02-01-mozilla-central/b2g-34.0a1.multi.linux-x86_64.tar.bz2" 2015-02-05 12:07:17 URL:http://ftp.mozilla.org/pub/mozilla.org/b2g/nightly/2014/08/2014-08-12-04-02-01-mozilla-central/b2g-34.0a1.multi.linux-x86_64.tar.bz2 [80354733/80354733] -> "b2g-34.0a1.multi.linux-x86_64.tar.bz2" [1] tar xjf "b2g-34.0a1.multi.linux-x86_64.tar.bz2" -C "/home/anindya/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01" test -f /home/anindya/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01/b2g/xpcshell # Running make without using a dependency ensures that we can run # "make node_modules" with a custom NODE_MODULES_GIT_URL variable, and then # run another target without specifying the variable make modules.tar make[1]: Entering directory `/home/anindya/gaia' Downloading latest node_modules package. This may take several minutes... wget -c -nv https://github.com/mozilla-b2g/gaia-node-modules/tarball/d22a9100fd46b8387eb4a992d8378166fc5d1a5b &&\ mv d22a9100fd46b8387eb4a992d8378166fc5d1a5b "modules.tar" 2015-02-05 12:14:58 URL:https://codeload.github.com/mozilla-b2g/gaia-node-modules/legacy.tar.gz/d22a9100fd46b8387eb4a992d8378166fc5d1a5b [32913615/32913615] -> "d22a9100fd46b8387eb4a992d8378166fc5d1a5b" [1] make[1]: Leaving directory `/home/anindya/gaia' tar --wildcards --strip-components 1 -x -m -f modules.tar "mozilla-b2g-gaia-node-modules-*/node_modules" npm install && npm rebuild npm WARN package.json mail-fakeservers@0.0.24 No README data npm WARN package.json marionette-client@1.7.0 No repository field. npm WARN package.json mocha-json-proxy@0.0.7 No repository field. npm WARN package.json dateformat@1.0.6-1.2.3 No repository field. npm WARN package.json marionette-content-script@0.0.4 No repository field. npm WARN package.json marionette-device-host@0.1.0 No repository field. npm WARN package.json marionette-file-manager@0.0.4 No repository field. npm WARN package.json marionette-profile-builder@0.0.3 No repository field. npm WARN package.json mocha-parallel@0.1.0 No repository field. > sockit-to-me@0.2.3 install /home/anindya/gaia/node_modules/marionette-client/node_modules/sockit-to-me > ./tools/copy.js || node-gyp configure build make[1]: Entering directory `/home/anindya/gaia/node_modules/marionette-client/node_modules/sockit-to-me/build' CXX(target) Release/obj.target/sockit/src/addon.o CXX(target) Release/obj.target/sockit/src/sockit.o SOLINK_MODULE(target) Release/obj.target/sockit.node SOLINK_MODULE(target) Release/obj.target/sockit.node: Finished COPY Release/sockit.node make[1]: Leaving directory `/home/anindya/gaia/node_modules/marionette-client/node_modules/sockit-to-me/build' > websocket@1.0.17 install /home/anindya/gaia/node_modules/marionette-js-runner/node_modules/marionette-js-logger/node_modules/websocket > (node-gyp rebuild 2> builderror.log) || (exit 0) marionette-client@1.5.8 node_modules/marionette-client ├── socket-retry-connect@0.0.1 ├── debug@0.6.0 ├── json-wire-protocol@0.2.1 (eventemitter2@0.4.14) └── sockit-to-me@0.2.3 marionette-js-runner@0.6.0 node_modules/marionette-js-runner ├── escape-string-regexp@1.0.2 ├── intersect@0.0.3 ├── array-difference@0.0.1 ├── commander@1.3.2 (keypress@0.1.0) ├── marionette-b2gdesktop-host@0.8.1 (mozilla-runner@0.5.0) └── marionette-js-logger@0.1.4 (websocket@1.0.17) > ffi-prebuilt@1.2.9 install /home/anindya/gaia/node_modules/ffi-prebuilt > ./tools/copy.js || node-gyp configure build make[1]: Entering directory `/home/anindya/gaia/node_modules/ffi-prebuilt/build' CC(target) Release/obj.target/ffi/deps/libffi/src/prep_cif.o CC(target) Release/obj.target/ffi/deps/libffi/src/types.o CC(target) Release/obj.target/ffi/deps/libffi/src/raw_api.o CC(target) Release/obj.target/ffi/deps/libffi/src/java_raw_api.o CC(target) Release/obj.target/ffi/deps/libffi/src/closures.o ../deps/libffi/src/closures.c: In function 'dlmmap_locked': ../deps/libffi/src/closures.c:421:17: warning: ignoring return value of 'ftruncate', declared with attribute warn_unused_result [-Wunused-result] ftruncate (execfd, offset); ^ ../deps/libffi/src/closures.c:433:17: warning: ignoring return value of 'ftruncate', declared with attribute warn_unused_result [-Wunused-result] ftruncate (execfd, offset); ^ CC(target) Release/obj.target/ffi/deps/libffi/src/x86/ffi.o CC(target) Release/obj.target/ffi/deps/libffi/src/x86/ffi64.o ../deps/libffi/src/x86/ffi64.c: In function 'classify_argument': ../deps/libffi/src/x86/ffi64.c:181:18: warning: suggest braces around empty body in an 'else' statement [-Wempty-body] FFI_ASSERT (0); ^ CC(target) Release/obj.target/ffi/deps/libffi/src/x86/unix64.o CC(target) Release/obj.target/ffi/deps/libffi/src/x86/sysv.o AR(target) Release/obj.target/deps/libffi/libffi.a COPY Release/libffi.a CXX(target) Release/obj.target/ffi_bindings/src/ffi.o CXX(target) Release/obj.target/ffi_bindings/src/callback_info.o CXX(target) Release/obj.target/ffi_bindings/src/threaded_callback_invokation.o SOLINK_MODULE(target) Release/obj.target/ffi_bindings.node SOLINK_MODULE(target) Release/obj.target/ffi_bindings.node: Finished COPY Release/ffi_bindings.node make[1]: Leaving directory `/home/anindya/gaia/node_modules/ffi-prebuilt/build' > ref-prebuilt@0.1.3 install /home/anindya/gaia/node_modules/ffi-prebuilt/node_modules/ref-prebuilt > ./tools/copy.js || node-gyp configure build make[1]: Entering directory `/home/anindya/gaia/node_modules/ffi-prebuilt/node_modules/ref-prebuilt/build' CXX(target) Release/obj.target/binding/src/binding.o SOLINK_MODULE(target) Release/obj.target/binding.node SOLINK_MODULE(target) Release/obj.target/binding.node: Finished COPY Release/binding.node make[1]: Leaving directory `/home/anindya/gaia/node_modules/ffi-prebuilt/node_modules/ref-prebuilt/build' > sockit-to-me@0.2.3 install /home/anindya/gaia/node_modules/marionette-client/node_modules/sockit-to-me > ./tools/copy.js || node-gyp configure build make[1]: Entering directory `/home/anindya/gaia/node_modules/marionette-client/node_modules/sockit-to-me/build' make[1]: Nothing to be done for `all'. make[1]: Leaving directory `/home/anindya/gaia/node_modules/marionette-client/node_modules/sockit-to-me/build' > dtrace-provider@0.2.8 install /home/anindya/gaia/node_modules/restify/node_modules/dtrace-provider > node-gyp rebuild make[1]: Entering directory `/home/anindya/gaia/node_modules/restify/node_modules/dtrace-provider/build' TOUCH Release/obj.target/DTraceProviderStub.stamp make[1]: Leaving directory `/home/anindya/gaia/node_modules/restify/node_modules/dtrace-provider/build' > sockit-to-me@0.2.3 install /home/anindya/gaia/node_modules/marionette-plugin-forms/node_modules/marionette-client/node_modules/sockit-to-me > ./tools/copy.js || node-gyp configure build make[1]: Entering directory `/home/anindya/gaia/node_modules/marionette-plugin-forms/node_modules/marionette-client/node_modules/sockit-to-me/build' CXX(target) Release/obj.target/sockit/src/addon.o CXX(target) Release/obj.target/sockit/src/sockit.o SOLINK_MODULE(target) Release/obj.target/sockit.node SOLINK_MODULE(target) Release/obj.target/sockit.node: Finished COPY Release/sockit.node make[1]: Leaving directory `/home/anindya/gaia/node_modules/marionette-plugin-forms/node_modules/marionette-client/node_modules/sockit-to-me/build' > ws@0.4.20 install /home/anindya/gaia/node_modules/test-agent/node_modules/websocket.io/node_modules/ws > node install.js npm ERR! weird error 1 npm WARN This failure might be due to the use of legacy binary "node" npm WARN For further explanations, please read /usr/share/doc/nodejs/README.Debian npm ERR! not ok code 0 make: *** [node_modules] Error 1
Flags: needinfo?(timdream)
> npm WARN This failure might be due to the use of legacy binary "node" It looks like you need to update your Node.js to the latest version. What's your node --version ? I am getting v0.10.35 on machine.
Flags: needinfo?(timdream)
anindya@anindya-virtual-machine:~/gaia$ node --version v0.10.35 anindya@anindya-virtual-machine:~/gaia$ make build-test-integration TEST_FILES=apps/keyboard/test/build/integration/keyboard_test.js Test SDK directory: /home/anindya/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01 test -f /home/anindya/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01/b2g/xpcshell run-js-command gaia/build-test [cmd] mocha --harmony --reporter spec --ui tdd --timeout 600000 /home/anindya/gaia/apps/keyboard/test/build/integration/keyboard_test.js Keyboard layouts building tests ✓ APP=keyboard make (111379ms) 1) APP=keyboard GAIA_KEYBOARD_LAYOUTS=* GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS=* make ✓ APP=keyboard GAIA_KEYBOARD_LAYOUTS=noPreloadDictRequired GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS="" make (15708ms) ✓ APP=keyboard GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS=en make (3261ms) Keyboard settings building tests For handwriting ✓ APP=keyboard make (3527ms) ✓ GAIA_KEYBOARD_LAYOUTS=zh-Hans-Handwriting APP=keyboard make (2178ms) User dictionary ✓ APP=keyboard make (3440ms) ✓ GAIA_KEYBOARD_ENABLE_USER_DICT=1 APP=keyboard make (3259ms) 7 passing (4m) 1 failing 1) Keyboard layouts building tests APP=keyboard GAIA_KEYBOARD_LAYOUTS=* GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS=* make: Uncaught Error: ENOENT, no such file or directory '/home/anindya/gaia/apps/keyboard/js/layouts/undefined.js' at Object.fs.statSync (fs.js:696:18) at Object.checkFileInZip (/home/anindya/gaia/build/test/integration/helper.js:78:17) at /home/anindya/gaia/apps/keyboard/test/build/integration/keyboard_test.js:161:16 at Array.forEach (native) at /home/anindya/gaia/apps/keyboard/test/build/integration/keyboard_test.js:160:17 at ChildProcess.exithandler (child_process.js:656:7) at ChildProcess.emit (events.js:98:17) at maybeClose (child_process.js:766:16) at Process.ChildProcess._handle.onexit (child_process.js:833:5) make: *** [build-test-integration] Error 1
I just managed to install node v0.10.35 and now I don't understand what's wrong
Flags: needinfo?(timdream)
(In reply to Anindya-Pandey from comment #23) > I just managed to install node v0.10.35 and now I don't understand what's > wrong I don't know what's wrong either. Sorry about the frustrations. The error was traced back to https://github.com/mozilla-b2g/gaia/blob/9d40b6e7b3f/apps/keyboard/test/build/integration/keyboard_test.js#L161 and it failed because of we attempt to read a file from your disk in /home/anindya/gaia/apps/keyboard/js/layouts/undefined.js Judging by the format, the path should be constructed here: https://github.com/mozilla-b2g/gaia/blob/9d40b6e7b3f/apps/keyboard/test/build/integration/keyboard_test.js#L105 With layoutIds come from here: https://github.com/mozilla-b2g/gaia/blob/9d40b6e7b3f/apps/keyboard/test/build/integration/keyboard_test.js#L95-L102 Yet strangely, even if something funny returned from fs.readdirSync(), we should have filter out in the path.extname() check. So I really don't know where the undefined come from. If you could insert some console.log() among these places and see what's the output, it would help. Try to dump |layouts|, |layoutIds| etc.
Flags: needinfo?(timdream)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #24) > (In reply to Anindya-Pandey from comment #23) > > I just managed to install node v0.10.35 and now I don't understand what's > > wrong > > I don't know what's wrong either. Sorry about the frustrations. > > The error was traced back to > https://github.com/mozilla-b2g/gaia/blob/9d40b6e7b3f/apps/keyboard/test/ > build/integration/keyboard_test.js#L161 > > and it failed because of we attempt to read a file from your disk in > /home/anindya/gaia/apps/keyboard/js/layouts/undefined.js > > Judging by the format, the path should be constructed here: > https://github.com/mozilla-b2g/gaia/blob/9d40b6e7b3f/apps/keyboard/test/ > build/integration/keyboard_test.js#L105 > > With layoutIds come from here: > https://github.com/mozilla-b2g/gaia/blob/9d40b6e7b3f/apps/keyboard/test/ > build/integration/keyboard_test.js#L95-L102 > > Yet strangely, even if something funny returned from fs.readdirSync(), we > should have filter out in the path.extname() check. So I really don't know > where the undefined come from. > > If you could insert some console.log() among these places and see what's the > output, it would help. Try to dump |layouts|, |layoutIds| etc. Sorry for all the trouble and thanks for all the help. I'll try to see and understand what's wrong.
I added console.log(layouts) 'js/layouts/ar.js', 'js/layouts/bg-BDS.js', 'js/layouts/undefined.js', 'js/layouts/bg-Pho-Ban.js', 'js/layouts/undefined.js', 'js/layouts/bg-Pho-Trad.js', 'js/layouts/undefined.js', 'js/layouts/bn-Avro.js', 'js/layouts/bn-Probhat.js', 'js/layouts/bs.js', 'js/layouts/ca.js', 'js/layouts/cs-qwerty.js', 'js/layouts/cs.js', 'js/layouts/da.js', 'js/layouts/de.js', 'js/layouts/dz-BT.js', 'js/layouts/el.js', 'js/layouts/undefined.js', 'js/layouts/en-Africa.js', 'js/layouts/en-Colemak.js', 'js/layouts/en-Dvorak.js', 'js/layouts/en-GB.js', 'js/layouts/en-Neo.js', 'js/layouts/en.js', 'js/layouts/eo.js', 'js/layouts/es-Americas.js', 'js/layouts/es.js', 'js/layouts/eu.js', 'js/layouts/ff.js', 'js/layouts/fr-CA.js', 'js/layouts/fr-CH.js', 'js/layouts/fr-Dvorak-bepo.js', 'js/layouts/fr.js', 'js/layouts/ga.js', 'js/layouts/gd.js', 'js/layouts/gl.js', 'js/layouts/gv.js', 'js/layouts/he.js', 'js/layouts/undefined.js', 'js/layouts/hi.js', 'js/layouts/hr.js', 'js/layouts/hu.js', 'js/layouts/it.js', 'js/layouts/jp-kanji.js', 'js/layouts/ko.js', 'js/layouts/undefined.js', 'js/layouts/lt.js', 'js/layouts/lv.js', 'js/layouts/mk.js', 'js/layouts/undefined.js', 'js/layouts/my.js', 'js/layouts/nb.js', 'js/layouts/nl.js', 'js/layouts/pl.js', 'js/layouts/pt-BR.js', 'js/layouts/pt-PT.js', 'js/layouts/ro.js', 'js/layouts/ru.js', 'js/layouts/undefined.js', 'js/layouts/sk.js', 'js/layouts/sq.js', 'js/layouts/sr-Cyrl.js', 'js/layouts/undefined.js', 'js/layouts/sr-Latn.js', 'js/layouts/sv.js', 'js/layouts/ta.js', 'js/layouts/te.js', 'js/layouts/th.js', 'js/layouts/tr-F.js', 'js/layouts/undefined.js', 'js/layouts/tr-Q.js', 'js/layouts/undefined.js', 'js/layouts/vi-Qwerty.js', 'js/layouts/undefined.js', 'js/layouts/vi-Telex.js', 'js/layouts/undefined.js', 'js/layouts/vi-Typewriter.js', 'js/layouts/undefined.js', 'js/layouts/wo.js', 'js/layouts/undefined.js', 'js/layouts/zh-Hans-Handwriting.js', 'js/layouts/undefined.js', 'js/layouts/zh-Hans-Pinyin.js', 'js/layouts/undefined.js', 'js/layouts/zh-Hant-Zhuyin.js' ]
Flags: needinfo?(timdream)
Could you console.log(layoutIds) or even console.log(filename) in the forEach loop? comment 26 is just a proof that the undefined string comes from |layouts|.
Flags: needinfo?(timdream)
I've just realized what's wrong with my code. So in here, I am using map() to get the layoutIds but I thought I was using forEach() (and follow by a push() to another array). In that case the early return block would work but with map() it would just return an undefined into the array. https://github.com/mozilla-b2g/gaia/blob/9d40b6e7b3f/apps/keyboard/test/build/integration/keyboard_test.js#L95-L102 To fix this, we should change readdirSync(...).map(...) into readdirSync(...).filter(...).map(...) and use Array#filter ( https://mdn.io/array%20filter ) to get the file names we want. If you could clone a bug and fix it that should unblock you from working this bug. Alternatively, you could just find that non-JS file in your local tree and remove it so you won't produce this bug. Sorry for waste your time on this silly mistake :'(.
Flags: needinfo?(anindyapandey)
anindya@anindya-virtual-machine:~/gaia$ make build-test-integration TEST_FILES=apps/keyboard/test/build/integration/keyboard_test.js Test SDK directory: /home/anindya/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01 test -f /home/anindya/gaia/b2g_sdk/34.0a1-2014-08-12-04-02-01/b2g/xpcshell run-js-command gaia/build-test [cmd] mocha --harmony --reporter spec --ui tdd --timeout 600000 /home/anindya/gaia/apps/keyboard/test/build/integration/keyboard_test.js /home/anindya/gaia/apps/keyboard/test/build/integration/keyboard_test.js~ Keyboard layouts building tests ✓ APP=keyboard make (4175ms) ✓ APP=keyboard GAIA_KEYBOARD_LAYOUTS=* GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS=* make (27995ms) ✓ APP=keyboard GAIA_KEYBOARD_LAYOUTS=noPreloadDictRequired GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS="" make (1484ms) ✓ APP=keyboard GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS=en make (2337ms) Keyboard settings building tests For handwriting ✓ APP=keyboard make (2558ms) ✓ GAIA_KEYBOARD_LAYOUTS=zh-Hans-Handwriting APP=keyboard make (1595ms) User dictionary ✓ APP=keyboard make (2490ms) ✓ GAIA_KEYBOARD_ENABLE_USER_DICT=1 APP=keyboard make (2594ms) Keyboard layouts building tests ✓ APP=keyboard make (4052ms) ✓ APP=keyboard GAIA_KEYBOARD_LAYOUTS=* GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS=* make (27436ms) ✓ APP=keyboard GAIA_KEYBOARD_LAYOUTS=noPreloadDictRequired GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS="" make (1184ms) ✓ APP=keyboard GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS=en make (2339ms) Keyboard settings building tests For handwriting ✓ APP=keyboard make (2504ms) ✓ GAIA_KEYBOARD_LAYOUTS=zh-Hans-Handwriting APP=keyboard make (1554ms) User dictionary ✓ APP=keyboard make (2571ms) ✓ GAIA_KEYBOARD_ENABLE_USER_DICT=1 APP=keyboard make (2463ms) 16 passing (1m) I made some changes in keyboard_test.js . Is this what I should expect?
Flags: needinfo?(anindyapandey) → needinfo?(timdream)
Yes, it's passing! But I would need to see the actual code to tell if the test is correct asserting the build results.
Flags: needinfo?(timdream)
'use strict'; /* jshint node: true, mocha: true */ /* global suiteSetup */ var assert = require('chai').assert; var path = require('path'); var fs = require('fs'); var helper = require('helper'); var AdmZip = require('adm-zip'); var jsdom = require('jsdom-nogyp').jsdom; suite('Keyboard layouts building tests', function() { suiteSetup(helper.cleanupWorkspace); teardown(helper.cleanupWorkspace); // default make -- build with selected layouts and dictionaries test('APP=keyboard make', function(done) { var cmd = 'APP=keyboard make'; helper.exec(cmd, function(error, stdout, stderr) { helper.checkError(error, stdout, stderr); var config = JSON.parse(process.env.BUILD_CONFIG); var layoutIds = config.GAIA_KEYBOARD_LAYOUTS.split(',').sort(); var zipPath = path.join(process.cwd(), 'profile', 'webapps', 'keyboard.gaiamobile.org', 'application.zip'); var appDirPath = config.GAIA_DIR + '/apps/keyboard'; var layouts = layoutIds.map(function(layout) { if(layout) return 'js/layouts/' + layout + '.js'; }); var layouts1 = layouts.map(function(layout) { return layout; }); layouts = layouts1; // Unfortunately we can only whilelist files here because we shouldn't // throw error if excluded files are missing (e.g. README). var imes = [ 'js/imes/jshangul/jshangul.js', 'js/imes/jspinyin/empinyin_files.data', 'js/imes/jspinyin/empinyin_files.js', 'js/imes/jspinyin/jspinyin.js', 'js/imes/jspinyin/libpinyin.js', 'js/imes/jspinyin/worker.js', 'js/imes/latin/latin.js', 'js/imes/latin/predictions.js', 'js/imes/latin/worker.js' ]; var dicts = [ 'js/imes/latin/dictionaries/de.dict', 'js/imes/latin/dictionaries/en_us.dict', 'js/imes/latin/dictionaries/es.dict', 'js/imes/latin/dictionaries/fr.dict', 'js/imes/latin/dictionaries/pl.dict', 'js/imes/latin/dictionaries/pt_br.dict' ]; var checkList = [].concat(layouts, imes, dicts); checkList.forEach(function(path) { helper.checkFileInZip(zipPath, path, appDirPath + '/' + path); }); // Verify inputs entry in manifest var zip = new AdmZip(zipPath); var entry = zip.getEntry('manifest.webapp'); var manifest = JSON.parse(zip.readAsText(entry)); var inputKeysInManifest = Object.keys(manifest.inputs); assert.deepEqual(inputKeysInManifest.sort(), [].concat(layoutIds, 'number').sort()); // Verify dictionary config var dictJSON = JSON.parse(fs.readFileSync( appDirPath + '/test/build/integration/resources/' + 'default-make-dictionaries.json')); helper.checkFileContentInZip( zipPath, 'js/settings/dictionaries.json', dictJSON, true); done(); }); }); // Build with all layouts and dictionaries test('APP=keyboard GAIA_KEYBOARD_LAYOUTS=* ' + 'GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS=* make', function(done) { var cmd = 'APP=keyboard GAIA_KEYBOARD_LAYOUTS=* ' + 'GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS=* make'; helper.exec(cmd, function(error, stdout, stderr) { helper.checkError(error, stdout, stderr); var config = JSON.parse(process.env.BUILD_CONFIG); var zipPath = path.join(process.cwd(), 'profile', 'webapps', 'keyboard.gaiamobile.org', 'application.zip'); var appDirPath = config.GAIA_DIR + '/apps/keyboard'; var layoutIds = fs.readdirSync(appDirPath + '/js/layouts').map(function(filename) { if (path.extname(filename) !== '.js') { return; } return path.basename(filename, '.js'); }); var layouts = layoutIds.map(function(layout) { if(layout) return 'js/layouts/' + layout + '.js'; }); var layouts1 = []; layouts.forEach(function(layout) { if(layout) layouts1.push(layout); }); layouts = layouts1; // Unfortunately we can only whilelist files here because we shouldn't // throw error if excluded files are missing (e.g. README). // For the sake of simplicity let's only check the main scripts. var imes = [ 'js/imes/handwriting/handwriting.js', 'js/imes/india/india.js', 'js/imes/jshangul/jshangul.js', 'js/imes/jskanji/jskanji.js', 'js/imes/jspinyin/jspinyin.js', 'js/imes/latin/latin.js', 'js/imes/vietnamese/vietnamese.js' ]; var dicts = [ 'js/imes/latin/dictionaries/bg.dict', 'js/imes/latin/dictionaries/bs.dict', 'js/imes/latin/dictionaries/ca.dict', 'js/imes/latin/dictionaries/cs.dict', 'js/imes/latin/dictionaries/da.dict', 'js/imes/latin/dictionaries/de.dict', 'js/imes/latin/dictionaries/el.dict', 'js/imes/latin/dictionaries/en_gb.dict', 'js/imes/latin/dictionaries/en_us.dict', 'js/imes/latin/dictionaries/es.dict', 'js/imes/latin/dictionaries/eu.dict', 'js/imes/latin/dictionaries/fr.dict', 'js/imes/latin/dictionaries/ga.dict', 'js/imes/latin/dictionaries/gd.dict', 'js/imes/latin/dictionaries/gl.dict', 'js/imes/latin/dictionaries/gv.dict', 'js/imes/latin/dictionaries/hr.dict', 'js/imes/latin/dictionaries/hu.dict', 'js/imes/latin/dictionaries/it.dict', 'js/imes/latin/dictionaries/lt.dict', 'js/imes/latin/dictionaries/lv.dict', 'js/imes/latin/dictionaries/nb.dict', 'js/imes/latin/dictionaries/nl.dict', 'js/imes/latin/dictionaries/pl.dict', 'js/imes/latin/dictionaries/pt_br.dict', 'js/imes/latin/dictionaries/pt_pt.dict', 'js/imes/latin/dictionaries/ro.dict', 'js/imes/latin/dictionaries/ru.dict', 'js/imes/latin/dictionaries/sk.dict', 'js/imes/latin/dictionaries/sq.dict', 'js/imes/latin/dictionaries/sr-Cyrl.dict', 'js/imes/latin/dictionaries/sr-Latn.dict', 'js/imes/latin/dictionaries/sv.dict', 'js/imes/latin/dictionaries/tr.dict' ]; var checkList = [].concat(layouts, imes, dicts); checkList.forEach(function(path) { helper.checkFileInZip(zipPath, path, appDirPath + '/' + path); }); // Verify inputs entry in manifest var zip = new AdmZip(zipPath); var entry = zip.getEntry('manifest.webapp'); var manifest = JSON.parse(zip.readAsText(entry)); var inputKeysInManifest = Object.keys(manifest.inputs); var layoutIds1 = []; layoutIds.forEach(function(layout) { if(layout) layoutIds1.push(layout); }); layoutIds=layoutIds1; assert.deepEqual(inputKeysInManifest.sort(), [].concat(layoutIds, 'number').sort()); // Verify dictionary config var dictJSON = JSON.parse(fs.readFileSync( appDirPath + '/test/build/integration/resources/' + 'all-layout-make-dictionaries.json')); helper.checkFileContentInZip( zipPath, 'js/settings/dictionaries.json', dictJSON, true); done(); }); }); // Build with all layouts with no dictionaries test('APP=keyboard GAIA_KEYBOARD_LAYOUTS=noPreloadDictRequired ' + 'GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS="" make', function(done) { var cmd = 'APP=keyboard GAIA_KEYBOARD_LAYOUTS=noPreloadDictRequired ' + 'GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS="" make'; helper.exec(cmd, function(error, stdout, stderr) { helper.checkError(error, stdout, stderr); var config = JSON.parse(process.env.BUILD_CONFIG); var zipPath = path.join(process.cwd(), 'profile', 'webapps', 'keyboard.gaiamobile.org', 'application.zip'); var appDirPath = config.GAIA_DIR + '/apps/keyboard'; // For this test, we verify there isn't any dictionary // in the zip. var zip = new AdmZip(zipPath); var entries = zip.getEntries(); var imePath = 'js/imes'; var sizeLimit = (1 << 10) * 100; // 100K entries.forEach(function(entry) { if (entry.entryName.substr(0, imePath.length) !== imePath) { return; } var fileSize = entry.getData().length; assert.isTrue(fileSize < sizeLimit, 'IME file is larger than non-dictionary limit: ' + entry.entryName + ', size: ' + fileSize + ' bytes.'); }); // Verify dictionary config var dictJSON = JSON.parse(fs.readFileSync( appDirPath + '/test/build/integration/resources/' + 'no-preload-dict-required-make-dictionaries.json')); helper.checkFileContentInZip( zipPath, 'js/settings/dictionaries.json', dictJSON, true); done(); }); }); // Build default layouts with only en dictionary, and extra IMEs test('APP=keyboard GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS=en make', function(done) { var cmd = 'APP=keyboard GAIA_KEYBOARD_PRELOAD_DICT_LAYOUTS=en make'; helper.exec(cmd, function(error, stdout, stderr) { helper.checkError(error, stdout, stderr); var config = JSON.parse(process.env.BUILD_CONFIG); var zipPath = path.join(process.cwd(), 'profile', 'webapps', 'keyboard.gaiamobile.org', 'application.zip'); var appDirPath = config.GAIA_DIR + '/apps/keyboard'; var layoutIds = config.GAIA_KEYBOARD_LAYOUTS.split(',').sort(); var layouts = layoutIds.map(function(layout) { if(layout) return 'js/layouts/' + layout + '.js'; }); var layouts1 = []; layouts.forEach(function(layout) { if(layout) layouts1.push(layout); }); layouts = layouts1; // Unfortunately we can only whilelist files here because we shouldn't // throw error if excluded files are missing (e.g. README). var imes = [ 'js/imes/jshangul/jshangul.js', 'js/imes/jspinyin/empinyin_files.data', 'js/imes/jspinyin/empinyin_files.js', 'js/imes/jspinyin/jspinyin.js', 'js/imes/jspinyin/libpinyin.js', 'js/imes/jspinyin/worker.js', 'js/imes/latin/latin.js', 'js/imes/latin/predictions.js', 'js/imes/latin/worker.js' ]; var dicts = [ 'js/imes/latin/dictionaries/es.dict', 'js/imes/latin/dictionaries/fr.dict', 'js/imes/latin/dictionaries/pl.dict', 'js/imes/latin/dictionaries/pt_br.dict' ]; var checkList = [].concat(layouts, imes); checkList.forEach(function(path) { helper.checkFileInZip(zipPath, path, appDirPath + '/' + path); }); // Verify inputs entry in manifest var zip = new AdmZip(zipPath); var entry = zip.getEntry('manifest.webapp'); var manifest = JSON.parse(zip.readAsText(entry)); var inputKeysInManifest = Object.keys(manifest.inputs); // Only layouts with dictionaries should be declaried. assert.deepEqual(inputKeysInManifest.sort(), ['en', 'ko', 'number', 'zh-Hans-Pinyin']); // Verify dictionaries are not built (except en_us.dict) dicts.forEach(function(dict) { var entry = zip.getEntry(dict); assert.equal(entry, null, 'Dictionary should not be built: ' + dict); }); // Verify dictionary config var dictJSON = JSON.parse(fs.readFileSync( appDirPath + '/test/build/integration/resources/' + 'default-make-en-dict-dictionaries.json')); helper.checkFileContentInZip( zipPath, 'js/settings/dictionaries.json', dictJSON, true); done(); }); }); }); suite('Keyboard settings building tests', function() { suiteSetup(helper.cleanupWorkspace); teardown(helper.cleanupWorkspace); // parse settings.html and return domDoc. var getSettingsDomDoc = function() { var zipPath = path.join(process.cwd(), 'profile', 'webapps', 'keyboard.gaiamobile.org', 'application.zip'); // Verify settings.html content in manifest var zip = new AdmZip(zipPath); var entry = zip.getEntry('settings.html'); return jsdom(zip.readAsText(entry)); }; // return an array of <scripts> tag in <head> var getScriptsFromDomDoc = function(domDoc) { // We don't have Array.from in our node version, so use an old way to // convert HTMLCollections to array return Array.prototype.slice.call( domDoc.head.getElementsByTagName('script')); }; suite('For handwriting', function() { // return an array of <sections> in the general panel var getSectionsFromGeneralPanel = function(domDoc) { return Array.prototype.slice.call( domDoc.querySelectorAll('#general-container > section')); }; // default: there shouldn't be handwriting elements in resulting file test('APP=keyboard make', function(done) { var cmd = 'APP=keyboard make'; helper.exec(cmd, function(error, stdout, stderr) { helper.checkError(error, stdout, stderr); var settingsDOMDoc = getSettingsDomDoc(); assert.isTrue(getScriptsFromDomDoc(settingsDOMDoc).every(function(elem){ return elem.src !== 'js/settings/handwriting_settings_view.js'; }), 'No script should include handwriting_settings_view.js'); assert.isTrue( getSectionsFromGeneralPanel(settingsDOMDoc).every(function(elem){ return elem.id !== 'handwriting-settings'; }), 'No section in general panel should include handwriting settings'); done(); }); }); test('GAIA_KEYBOARD_LAYOUTS=zh-Hans-Handwriting APP=keyboard make', function(done) { var cmd = 'GAIA_KEYBOARD_LAYOUTS=zh-Hans-Handwriting APP=keyboard make'; helper.exec(cmd, function(error, stdout, stderr) { helper.checkError(error, stdout, stderr); var settingsDOMDoc = getSettingsDomDoc(); assert.isTrue(getScriptsFromDomDoc(settingsDOMDoc).some(function(elem){ return elem.src === 'js/settings/handwriting_settings_view.js'; }), 'Some script should include handwriting_settings_view.js'); assert.isTrue( getSectionsFromGeneralPanel(settingsDOMDoc).some(function(elem){ return elem.id === 'handwriting-settings'; }), 'Some section in general panel should include handwriting settings'); done(); }); }); }); suite('User dictionary', function() { // return an array of <li> in the root panel's first section's ui var getLIsFromGeneralPanel = function(domDoc) { return Array.prototype.slice.call( domDoc.querySelectorAll('#general-settings > ul > li')); }; // default: there shouldn't be user dictionary elements in resulting file test('APP=keyboard make', function(done) { var cmd = 'APP=keyboard make'; helper.exec(cmd, function(error, stdout, stderr) { helper.checkError(error, stdout, stderr); var settingsDOMDoc = getSettingsDomDoc(); assert.isTrue(getScriptsFromDomDoc(settingsDOMDoc).every(function(elem){ return elem.src !== 'js/settings/user_dictionary_edit_dialog.js'; }), 'No script should include user_dictionary_edit_dialog.js'); assert.isTrue(getScriptsFromDomDoc(settingsDOMDoc).every(function(elem){ return elem.src !== 'js/settings/user_dictionary_list_panel.js'; }), 'No script should include user_dictionary_list_panel.js'); assert.isTrue(getScriptsFromDomDoc(settingsDOMDoc).every(function(elem){ return elem.src !== 'js/settings/user_dictionary.js'; }), 'No script should include user_dictionary.js'); assert.isTrue( getLIsFromGeneralPanel(settingsDOMDoc).every(function(elem){ return elem.querySelector('a#menu-userdict') === null; }), 'No <li> in general panel should include user dict settings'); done(); }); }); test('GAIA_KEYBOARD_ENABLE_USER_DICT=1 APP=keyboard make', function(done) { var cmd = 'GAIA_KEYBOARD_ENABLE_USER_DICT=1 APP=keyboard make'; helper.exec(cmd, function(error, stdout, stderr) { helper.checkError(error, stdout, stderr); var settingsDOMDoc = getSettingsDomDoc(); assert.isTrue(getScriptsFromDomDoc(settingsDOMDoc).some(function(elem){ return elem.src === 'js/settings/user_dictionary_edit_dialog.js'; }), 'Some script should include user_dictionary_edit_dialog.js'); assert.isTrue(getScriptsFromDomDoc(settingsDOMDoc).some(function(elem){ return elem.src === 'js/settings/user_dictionary_list_panel.js'; }), 'Some script should include user_dictionary_list_panel.js'); assert.isTrue(getScriptsFromDomDoc(settingsDOMDoc).some(function(elem){ return elem.src === 'js/settings/user_dictionary.js'; }), 'Some script should include user_dictionary.js'); assert.isTrue( getLIsFromGeneralPanel(settingsDOMDoc).some(function(elem){ return elem.querySelector('a#menu-userdict') !== null; }), 'Some <li> in general panel should include user dict settings'); done(); }); }); }); });
As I said in comment 28, could you file another bug and fix the test code mistake there, with Array#filter?
Got it. Just help me with the topic of the bug please.
Flags: needinfo?(timdream)
(In reply to Anindya-Pandey from comment #33) > Got it. Just help me with the topic of the bug please. Per conversation on IRC a week ago, instead of render.js you should look at LayoutPageView and make sure it set the lang attribute to the container element from information got from layout pages. Please ask if you have anything specific.
Flags: needinfo?(timdream)
So we need to add a |lang| field in the layout files. Then we set the lang attribute of the container element to the |lang| field of the layout file. Am I right? The var |container| holds the container element. How to access the lang attribute of the container element using the var |container|? Also thanks for all the help on the previous bug.
Flags: needinfo?(timdream)
(In reply to Anindya-Pandey from comment #35) > So we need to add a |lang| field in the layout files. > Then we set the lang attribute of the container element to the |lang| field > of the layout file. > Am I right? > Yes, that's exactly what to do here. Please be reminded you'll need to look into LayoutLoader LayoutManager etc to see how the lang property will be exposed to LayoutPageView. The code path is pretty long since this is a complex app. > The var |container| holds the container element. How to access the lang > attribute of the container element using the var |container|? The container element is created by the LayoutPageView instance based on the currentPage JSON structure it received. > Also thanks for all the help on the previous bug. No problem, thank you for fixing it!
Flags: needinfo?(timdream)
Hey Tim I've been going through many of the files. Here's what I understand. Tell me if I'm going in the right way. So, In layout_manager.js in _updateCurrentPage() var layout = this.loader.getLayout(typeSpecificLayoutName || this._typeGenericLayoutName); // I think |layout| receives the layout here. var page = layout.pages[this.currentPageIndex]; if (!page) { var defaultLayout = this.loader.getLayout(this.DEFAULT_LAYOUT_NAME); page = defaultLayout.pages[this.currentPageIndex]; } // |page| receives the current layout page page = this.currentPage = Object.create(page); // |page| points to |currentPage| ['imEngine', 'autoCorrectLanguage', 'autoCorrectPunctuation', 'needsCandidatePanel' ].forEach(function(prop) { if (prop in layout) { page[prop] = layout[prop]; } }); //|page| receives properties from |layout|. I think the lang property can be added here. I think the changes in |page| will reflect in |currentPage|. In layout_rendering_manager.js in updateLayoutRendering() var currentPage = this._currentRenderingPage = this.app.layoutManager.currentPage; //|currentPage| points to |layoutManager.currentPage| var p = new Promise(function(resolve) { this.app.viewManager.render(currentPage, { uppercase: this.app.upperCaseStateManager.isUpperCase, inputType: this.app.getBasicInputType(), showCandidatePanel: needsCandidatePanel }, resolve); }.bind(this)).then(this._afterRenderDrew.bind(this)); //|currentPage| ie |layoutManager.currentPage| is passed to |viewManager.render(layout, flags, callback)| In view_manager.js in render(layout, flags, callback) //layout receives |currentPage| pageView = new LayoutPageView(layout, options, this); in layout_page_view.js in LayoutPageView(layout, options, viewManager) //|layout| receives the |layout| that was defined in view_manager.js this.layout = layout; //ultimately |this.layout| receives |layout|. So I think that if we add the lang property to |page| in layout_manager.js then |currentPage| would have access to the lang property and it would be exposed to layout_view_Manager.js through |layout|. Sorry it took so long. This is indeed a complex app and I have been busy with my exams.
Flags: needinfo?(timdream)
Your comment is entirely correct. You could either add the |lang| property (*and* add the property to the array below to cause the value to be copied from the layout object to the page object), or you could add the property in page object directly. Either way, LayoutPageView instance would receive the lang value. (In reply to Anindya-Pandey from comment #37) > // |page| points to |currentPage| > > ['imEngine', 'autoCorrectLanguage', > 'autoCorrectPunctuation', 'needsCandidatePanel' > ].forEach(function(prop) { > if (prop in layout) { > page[prop] = layout[prop]; > } > }); > > //|page| receives properties from |layout|. I think the lang property can be > added here. I think the changes in |page| will reflect in |currentPage|. ... and yes, the |layout| in |new LayoutPageView(layout, options, this);| is actually pointing to |page| object holds in LayoutManager#currentPage. Sorry for the naming confusion -- I should have caught it when I review the patch before. If that bother you a lot you can file a bug to fix that.
Flags: needinfo?(timdream)
Could you tell me where I can find lang attribute values for the different layouts ? I realised that the for many of the layouts the lang attribute value is the same as autoCorrectLanguage field value or shortLabel field value. But I'm not sure. So it would help if you could point me to a resource.
Flags: needinfo?(timdream)
(In reply to Anindya-Pandey from comment #39) > Could you tell me where I can find lang attribute values for the different > layouts ? > I realised that the for many of the layouts the lang attribute value is the > same as autoCorrectLanguage field value or shortLabel field value. > But I'm not sure. > So it would help if you could point me to a resource. I don't have a definite list, but basically, yes, the value should be the locale code of the language/region, which the auto-correct language dict files is also based on.
Flags: needinfo?(timdream)
I have added the lang attribute to all layouts and added the 'lang' property to ['imEngine', 'autoCorrectLanguage', 'autoCorrectPunctuation', 'needsCandidatePanel']. What next Tim?
Flags: needinfo?(timdream)
As previously explained, make LayoutPageView can pick up the |lang| value and set it to the container element of the layout.
Flags: needinfo?(timdream)
['imEngine', 'autoCorrectLanguage', > 'autoCorrectPunctuation', 'needsCandidatePanel', 'lang' > ].forEach(function(prop) { > if (prop in layout) { > page[prop] = layout[prop]; > } > }); shouldn't this expose the 'lang' value to LayoutPageView through |layout| that is passed in LayoutPageView(layout, options, viewManager)?
Flags: needinfo?(timdream)
(In reply to Anindya-Pandey from comment #43) > shouldn't this expose the 'lang' value to LayoutPageView through |layout| > that is passed in LayoutPageView(layout, options, viewManager)? It should. What's your finding here?
Flags: needinfo?(timdream)
I was just making sure. Does |container| in render() hold the container element?
Flags: needinfo?(timdream)
(In reply to Anindya-Pandey from comment #45) > I was just making sure. > Does |container| in render() hold the container element? Yes.
Flags: needinfo?(timdream)
OK, so container.setAttribute("lang",layout.lang) should do it?
Flags: needinfo?(timdream)
(In reply to Anindya-Pandey from comment #47) > OK, so container.setAttribute("lang",layout.lang) should do it? Yes, I think so.
Flags: needinfo?(timdream)
Comment on attachment 8577958 [details] added lang field to all layouts, exposed lang to LayoutPageView, set lang attribute of container to layout.lang This is great! Thank you very much. I took some time to look up the specs: according to the HTML5 spec [1], lang attribute should be BCP47 tags [2]. The first thing to note is that the separators should be dash ("-") instead of underlines ("_"), so I mark some of that in your patch. The next thing to fix is to make all country/region code upper case (e.g. "GB", "US", but not "Hant" or "Hans"). A great guideline [4] is published on W3C on how to choose the right BCP47 tag, but I don't think we need to be using all subtags to achieve absolute specificity. As long as the tag is specific enough to tell the browser which font it should use to display the content, we should be fine. The desired effect of the lang tag, for us (the web developers), is to make sure the browser use the right font to display a script/script, or script variant that share the same Unicode code range with other language/script. For example, the Han characters should be shown differently across languages even though the character is on the same code point [5]. [1] https://html.spec.whatwg.org/multipage/dom.html#the-lang-and-xml:lang-attributes [2] http://www.ietf.org/rfc/bcp/bcp47.txt , with it's history described in [3] [3] https://en.wikipedia.org/wiki/IETF_language_tag [4] http://www.w3.org/International/questions/qa-choosing-language-tags [5] http://googledevelopers.blogspot.tw/2014/07/noto-cjk-font-that-is-complete.html see the 3rd figure and the description above it.
Attachment #8577958 - Flags: review?(timdream) → feedback+
I made the changes. Should I also make changes to autoCorrectLanguage fields? Please have look and tell me if I need to change something.
Flags: needinfo?(timdream)
Comment on attachment 8577958 [details] added lang field to all layouts, exposed lang to LayoutPageView, set lang attribute of container to layout.lang (In reply to Anindya-Pandey from comment #52) > I made the changes. Should I also make changes to autoCorrectLanguage > fields? > Please have look and tell me if I need to change something. You don't need to make changes to the `autoCorrectLanguage` field. It's an id referencing the language dictionary file to load. I am setting r+ now because this is entirely correct. Thanks! Please fix the final nit there and set checkin-needed on the bug to get this checked-in.
Flags: needinfo?(timdream)
Attachment #8577958 - Flags: review+
I made the changes. Please have a look and tell me if I need to change something. Thank You so much for all the help on the bug.
Flags: needinfo?(timdream)
Flags: a11y-review-
Keywords: checkin-needed
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/pull/28888 Autolander could not land the pull request due to not having collaborator rights. This is possibly due to a tree closure. Please check the tree status and request checkin again once the tree is open.
I'm sorry I don't know how I set the a11y-review- flag. I don't know how to clear it.
Keywords: checkin-needed
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/pull/28888 Autolander could not land the pull request due to not having collaborator rights. This is possibly due to a tree closure. Please check the tree status and request checkin again once the tree is open.
Keywords: checkin-needed
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/pull/28888 Autolander could not land the pull request due to not having collaborator rights. This is possibly due to a tree closure. Please check the tree status and request checkin again once the tree is open.
It is appear we need to wait for tree to reopen.
Flags: needinfo?(timdream)
Flags: a11y-review-
Keywords: checkin-needed
Keywords: checkin-needed
http://docs.taskcluster.net/tools/task-graph-inspector/#docmkjxtRwyTMEFnFxQc2Q The pull request failed to pass integration tests. It could not be landed, please try again.
What exactly will I need to change?
Flags: needinfo?(anindyapandey) → needinfo?(timdream)
Really sorry for the late reply. I will get back to this bug as soon as I have time to help.
Attachment #8577952 - Attachment is obsolete: true
Flags: needinfo?(timdream)
Comment on attachment 8577958 [details] added lang field to all layouts, exposed lang to LayoutPageView, set lang attribute of container to layout.lang I've just created a new pull request to land your patch, with my follow-up fix to the tests. I think I set the test up in the wrong way ... I should simply it in another bug. Do you want to help?
Attachment #8577958 - Attachment is obsolete: true
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: